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

Remove class_inactive option #14

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

sn3p
Copy link

@sn3p sn3p commented Mar 3, 2020

closes #10

This removes class_inactive in favour of class as discussed here.

This doesn't introduce any new options like the proposed wrap_* classes, because I found there already is an undocumented wrap_tag_opts option which kinda solves the issue.

@danhper what do you think?
I'm willing to take this further, but before I do I like to know your opinion on this.


  • Tests
  • Remove :class_inactive in favour of :class
  • Include wrap_tag_opts option in the docs
  • Update readme, docs and examples
  • Only add active class to wrap_tag or link (not both) ?

Examples

active_link(conn, "Home", to: "/", class: "link", class_active: "enabled")
# <a class="enabled link" href="/">Home</a>
active_link(conn, "Home", to: "/", class: "nav-link", wrap_tag: :li, wrap_tag_opts: [class: "nav-item"])
# <li class="active nav-item">
#   <a class="active nav-link" href="/">Home</a>
# </li>

@danhper
Copy link
Owner

danhper commented Mar 9, 2020

Hi and thanks a lot for all the work.
I had completely forgotten about wrap_tag_opts.
I feel it would make more sense to still have wrap_class_active to avoid having the active class on both elements.
What do you think?

@sn3p
Copy link
Author

sn3p commented Mar 11, 2020

I had completely forgotten about wrap_tag_opts.
I feel it would make more sense to still have wrap_class_active to avoid having the active class on both elements.

@danhper wouldn't it make sense to just have class_active and assign it to the wrap_tag instead of the link? This way we keep a single option to define the active class, and would probably be less of a breaking change in the next release.

Examples

active_link(conn, "Home", to: "/")
# <a class="active" href="/">Home</a>

active_link(conn, "Home", to: "/", wrap_tag: :li)
# <li class="active">
#   <a href="/">Home</a>
# </li>

active_link(conn, "Home", to: "/", wrap_tag: :li, class_active: "enabled")
# <li class="enabled">
#   <a href="/">Home</a>
# </li>

The wrap_tag_opts is still handy when you want more control over the wrapper:

active_link(conn, "Home",
  to: "/",
  class: "nav-link",
  wrap_tag: :li,
  wrap_tag_opts: [class: "nav-item"]
)
# <li class="active nav-item">
#   <a class="nav-link" href="/">Home</a>
# </li>

active_link(conn, "Home",
  to: "/",
  class: "nav-link",
  class_active: "enabled",
  wrap_tag: :li,
  wrap_tag_opts: [class: "nav-item"]
)
# <li class="enabled nav-item">
#   <a class="nav-link" href="/">Home</a>
# </li>

@sn3p
Copy link
Author

sn3p commented Mar 20, 2020

@danhper please let me know what you think about my comment above.

@danhper
Copy link
Owner

danhper commented Mar 20, 2020

@sn3p Thanks for the proposal and sorry for the delay.

wouldn't it make sense to just have class_active and assign it to the wrap_tag instead of the link?

I think this is indeed simpler and should be enough to support any CSS framework so let's go with that. Thanks again!

@Matsa59
Copy link
Collaborator

Matsa59 commented Oct 5, 2021

What is the state of this PR? does it need some job tbd ?

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

Successfully merging this pull request may close these issues.

Add ability to set different tags for wrapper vs link
3 participants