Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add active class page helpers #969

Open
paulcsmith opened this issue Nov 18, 2019 · 3 comments
Open

Add active class page helpers #969

paulcsmith opened this issue Nov 18, 2019 · 3 comments

Comments

@paulcsmith
Copy link
Member

paulcsmith commented Nov 18, 2019

https://gitter.im/luckyframework/Lobby?at=5dd31ca84adf071a8453ee00

Just an example

# src/pages/helpers/active_link.cr
module Helpers::ActiveLink
  def active_link_if(active? : Bool, text, to : Lucky::RouteHelper | Lucky::Action.class, **html_options)
    link(text, to, **add_active_class_if(active?, html_options))
  end

  def active_link_if(active? : Bool, to : Lucky::RouteHelper | Lucky::Action.class, **html_options)
    link(to, **add_active_class_if(active?, html_options)) do
      yield
    end
  end

  def active_link_if(active? : Bool, to : Lucky::RouteHelper | Lucky::Action.class, **html_options)
    link(to, **add_active_class_if(active?, html_options))
  end

  private def add_active_class_if(active? : Bool, html_options)
    if active? && html_options[:class]
      html_options = html_options.merge({class: html_options[:class] + " is-active"})
    elsif active?
      html_options = html_options.merge({class: "is-active"})
    end
    html_options
  end
end

# src/pages/helpers/html_builder.cr
require "./active_link.cr"

module Lucky::HTMLBuilder

We should think this through, but this or some version of it should definitely be built-in to Lucky

Maybe this could be simplified?

def class_if(condition : Bool, class html_class : String)
  html_class if condition
end

link "User list",m Users::Index, class: "bg-gray #{class_if(true, "bg-red")}"
@bdtomlin
Copy link
Contributor

I don't really think

link "User list", Users::Index, class: %(bg-gray #{class_if(true, "bg-red")})

is an improvement over

link "User list", Users::Index, class: %(bg-gray #{"bg-red" if true})

for a couple of reasons...

  1. It's more code.
  2. It doesn't offer any abstraction. At least when considering the example of active link if the active link class changes it only has to be changed in one place vs changing every class: "#{class_if(true, "is-active")}" in the application.

@paulcsmith
Copy link
Member Author

@bdtomlin You're totally right about if true being just as clear and shorter!

Regarding 2 I think it depends on what CSS you are writing. I've been using Tailwind so each active link tends to be different bg-grey text-white etc, but yes if you use is-active then a helper method would be better. I'm thinking for that maybe we add a guide or something since this seems very application specific.

But maybe we have some built-in helpers for current_page? or something. Similar to Rails. That could help with the link helpers https://apidock.com/rails/ActionView/Helpers/UrlHelper/current_page%3F

@bdtomlin
Copy link
Contributor

@paulcsmith, based on your reply, I agree that active_link_if and class_if are probably not the best examples of page helpers with which to initialize a new app. My original intent on this was not about an active class helper, but more about having page helpers in general and adding a default example so it was clear how to set up your own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants