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

added no_container option to link_to helper #114

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

added no_container option to link_to helper #114

wants to merge 2 commits into from

Conversation

Incanus3
Copy link

this way, one can manually create (and customize) the surrounding li container inside nav or dropdown and tell link_to not to create it

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 99.95% when pulling b476ab4 on Incanus3:master into f708ad0 on Fullscreen:master.

fixed side-efect of removing :no_container option from args
added spec
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5c1d6b5 on Incanus3:master into f708ad0 on Fullscreen:master.

@Incanus3
Copy link
Author

Btw interesting thing the

expect(:link_to).to generate html

Confused me for a moment, before I realized it's a custom matcher.

@claudiofullscreen
Copy link
Contributor

@Incanus3 before proceeding with this PR, could you show me a real example where you need this kind of customization? Thanks!

@claudiofullscreen
Copy link
Contributor

Also… I think this option should be on the wrapping nav selector, so you don't have to add it to each link_to.

@Incanus3
Copy link
Author

Incanus3 commented Feb 7, 2015

= nav do
  - categories.each do |category|
    %li{ class: active_if(selected_category == category) }
      = link_to category.capitalize, events_path(category: category), no_container: true

(active_if just returns 'active' if given true, '' otherwise)

and you're right, the option should probably be on nav, but the container is being added in the link_to helper, so it seemed like the easiest thing to do.

@claudiofullscreen
Copy link
Contributor

@Incanus3 Did you know that bh already sets the active class on the wrapping <li> if the link matches the current page? See http://fullscreen.github.io/bh/#links

@Incanus3
Copy link
Author

Incanus3 commented Feb 7, 2015

I do, but that's not flexible enough for my (and presumably other's) needs. The problem here is (among others), that when I first come to this page with category tabs, there's no category param, so the first tab is not highlighted. I cound of course add the param there, but that would mean that every link to that page needs to add the category param for it to be correctly highlighted.

@claudiofullscreen
Copy link
Contributor

If I understand correctly, your nav links are something like

  • /events?category=foo
  • /events?category=bar
  • /events?category=boo

and you would live one of the links to be active not because it is exactly the current page (i.e., because your browser URL is exactly /events?category=foo), but because somewhere else in the code you set a variable called selected_category to one of those values.

For instance, if you are in the page /events?category=foo and you have set your selected_category variable to bar, then you would want the second link to be active, not the first one. Is that right?

@Incanus3
Copy link
Author

ad 1st paragraph: exactly. ad 2nd one: I don't override the url param by setting selected_category elsewhere, but I may set selected_category, but keep the url /events

@Incanus3
Copy link
Author

also, I'd like the first tab to be default, but don't want every link_to to pass the first category, so if the param is missing entirely, I set selected_category to first category, but still need the tab to be highlighted.

@claudiofullscreen
Copy link
Contributor

Hello @Incanus3 – I think the case you are making is very custom.

You want to set the active class to a link that does not exactly match the current URL.

In the most general case, that is an "unexpected" behavior… the active class is meant to identify the current page.

You can definitely do that in your code, but I don't see this as something worth generalizing to the bh gem.

The solution for your case is quite simple: do not use the nav helper from Bh in that specific context, but simply write out the <ul nav ..><li>..</li><li>..</li></nav> elements the way you want them. Then, you will still be able to use link_to and the links won't be wrapped in a container.

Makes sense?

@Incanus3
Copy link
Author

Well, I'd agree that wanting the active class on a link that's realy different from the current page would be "unexpected". But I think that the latter case I mentioned (making the first tab default, thus '/events' ~= '/events?category=first_category') is quite common and in case of '/events' I need to highlight the first tab manually.

Also, I think this option is not that much added code for greater flexibility. Of course, your suggested solution would work, but the reason I use bh is exactly so I don't need to do this. It's not just that I need to write a few more characters, but the view is much more readable when using the helper.

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.

3 participants