Skip to content

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

Open
asalant wants to merge 2 commits intomasterfrom
jgravois-more-selected-nav-links
Open

Ensure that subroutes also display the selected nav, take 2#54
asalant wants to merge 2 commits intomasterfrom
jgravois-more-selected-nav-links

Conversation

@asalant
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@asalant
Copy link
Copy Markdown
Owner Author

asalant commented Jan 22, 2020 via email

@jgravois
Copy link
Copy Markdown
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