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

Ensure that subroutes also display the selected nav, take 2 #54

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

Conversation

asalant
Copy link
Owner

@asalant asalant commented Jan 22, 2020

@jgravois I checked out your PR to play with (and to try to remember Ruby!). I concluded that we were getting into enough tweaky logic that it was important to have tests. That took me down a rabbit hole of how to best test the helper methods.

Here's the end result. The changes are probably overkill but it was fun to relearn a bit.

The tab_item method is I think a bit clearer to understand, and it now has tests. These changes also change the Visits tab URL to just point at /visits which I think would have made the changes you were trying to make easier too.

@asalant asalant force-pushed the jgravois-more-selected-nav-links branch from 6ba1967 to a5d409c Compare January 22, 2020 00:55
Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very cool!

@jgravois
Copy link
Contributor

I forgot to mention, you're also welcome to just pile commits onto the PRs I already have open too.

@asalant
Copy link
Owner Author

asalant commented Jan 22, 2020 via email

@jgravois
Copy link
Contributor

jgravois commented Jan 22, 2020

yeah. I think I gave you permission manually, but its also just a handy (new) default in GitHub to give maintainers push access to the forked repo branch when a PR is opened.

Screen Shot 2020-01-22 at 1 09 16 PM

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.

2 participants