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

Paths (as hash) with different controllers but the same action name should not be highlighted? #42

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

Conversation

XA21X
Copy link

@XA21X XA21X commented Jun 4, 2014

The spec I wrote shows my interpretation of the expected behaviour of Item.highlights_on?. This is how the method is used when the :highlights_on option has not been set when Navigasmic::Item.new is called with the link specified as a hash.

I do not know whether my interpretation is correct but in aefed7c where this behaviour was previously modified, the change in line 38 (hash matching using OR logic instead of AND) of item.rb seems unnecessary for the implementation of the multi-controller feature (because they are in separate hashes).

I discovered this issue while I was using navigasmic to generate links to sign_in and sign_up of the Devise authentication gem. The link sign_in would be highlighted even though I was on the sign_up page.

Related to this, it seems like it would be useful in the future to add an optional parameter to semantic_navigation to specify the default behaviour of Item.highlights_on? so that for example, a navigation menu in the top bar could highlight on the same controller but a side menu would highlight only when the action also matches.

@shekibobo
Copy link
Contributor

Re: aefed7c, my main goal was to keep the logic that was originally there, which was attempting to detect any of the expected rules. However, the only case I was testing was for the controller key in multiple hashes. I don't think I considered that hashes would contain other keys to match on. This looks like a good change to me. Thanks for picking that up.

@mileszim
Copy link

+1

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