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
fix: only show active router link styles on parent page #22069
fix: only show active router link styles on parent page #22069
Conversation
Thanks for taking the time to open a PR!
|
:active="isActive" | ||
:active="isExactActive" |
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.
I opted for a global change instead of a Specs-only exception to ensure all navigation links continue to have the same behavior.
This means that if any of the other pages have links to a child view with a different URL (whether it be query params, pathname addition, etc) the navigation link will lose its active styles. This is intentional so that the user can more intuitively understand how to return to the parent page.
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.
Yeah I agree with doing this as the default. It's likely in the future, if we have nested router views, there would be cases where the exact route changes but the user is still "on" the main page. But I'm fine with not worrying about that till we get to that point.
Test summaryRun details
View run in Cypress Dashboard ➡️ Flakiness
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
cy.contains('.router-link-exact-active', 'Specs').should('not.exist') | ||
cy.percySnapshot() | ||
|
||
cy.get('a[href="#/specs"]').click() |
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.
does it make sense to use a more specific selector like cy.findAllByTestId('nav-link-spec')
just in case another link was added later?
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.
Yes, good call!
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.
The update looks elegant!
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.
Works great and feels much more clear 👍
User facing changelog
Minor UI tweak to better indicate how to return to the Specs list when the test runner is active
Additional details
During the bug hunt, several Cypress employees on a video call could not see how to get back to the specs list from the runner, which is fair since the link that navigates there is active, so it is not styled to be clickable (other links have cursor: pointer and hover states, this does not, since we are already inside that part of the app).
This change makes it so the navigation links are only styled as active when the URL is an exact match. Now, if the user goes to a child page (i.e. test runner is a child page of the Specs page) the link to the parent page will appear clickable as it no longer has active styles.
Steps to test
yarn dev
logger.cy.ts
How has the user experience changed?
Before:
After:
PR Tasks
cypress-documentation
?type definitions
?