luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.58k stars 157 forks source link

Add active class page helpers #969

Open paulcsmith opened 4 years ago

paulcsmith commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

@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.