-
Notifications
You must be signed in to change notification settings - Fork 18
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 past visits and reports subroutes also display the selected nav #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! See my review comment.
elsif label === 'Visits' | ||
"<li class='#{request.path.include?('/visits/') ? 'selected' : ''}'><a href='#{path}'>#{label}</a></li>" | ||
else | ||
"<li class='#{request.path.start_with?(path) ? 'selected' : ''}'><a href='#{path}'>#{label}</a></li>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this line or something similar should be all that is needed to replace the original request.path == path
check. Do we need the three checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we only use start_with
, 'Home' will also be selected when folks visit the Visits/Reports/Settings routes.
if I knew as much about ruby as I do about JS I could have condensed the other two by separating the root of the default 'Visits' path from today's date, but I took the easy way out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would probably work.
if label === 'Home' || label === 'Settings'
"<li class='#{request.path == path ? 'selected' : ''}'><a href='#{path}'>#{label}</a></li>"
else
"<li class='#{request.path.include?('/' + label.downcase + '/') ? 'selected' : ''}'><a href='#{path}'>#{label}</a></li>"
I'll find some time to make sure soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, AFAICT, the two pesky routes below make it pretty tough to cram all the logic you'd need in without a third clause.
/[org]/reports
/[/org]/reports/visits
if /[org]/reports/
was served up with a trailing slash, it'd be a different story.
edit: I'm a bit hindered here by the fact that I don't even know how to set breakpoints or inspect variables in Ruby/Rails.
if I knew how to do that, I'd imagine I could split the path up using a regex to isolate the portion of the route that comes immediately after the organization name and compare that to the label instead.
superceded by #54 |
just a small tweak to help the routes below display a selected nav option in the header.
/[org]/visits/2020/1/17
/[org]/visits/2020/1/16
(etc.)/[org]/reports/people
[/org]/reports/visits
(etc.)