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.

Sign up for GitHub

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

Jump to bottom

fix: only show active router link styles on parent page #22069

Merged
merged 8 commits into from
Jun 3, 2022
Merged

fix: only show active router link styles on parent page #22069

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
88f6e38
use isExactActive
rachelruderman Jun 1, 2022
abc2817
add tests
rachelruderman Jun 2, 2022
da29d38
Merge branch 'develop' into 21873/indicate-how-to-get-to-specs-from-r…
rachelruderman Jun 2, 2022
4327281
add testId for sidebar links
rachelruderman Jun 3, 2022
6c222ee
Use testId II
rachelruderman Jun 3, 2022
4c815fd
Use testId III
rachelruderman Jun 3, 2022
6945a3d
Merge branch 'develop' into 21873/indicate-how-to-get-to-specs-from-r…
rachelruderman Jun 3, 2022
934591e
move testId
rachelruderman Jun 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/app/cypress/e2e/sidebar_navigation.cy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,20 @@ describe('Sidebar Navigation', () => {
cy.get('.router-link-active').findByText('Specs').should('be.visible')
})

it('Specs sidebar nav link is not active when a test is running', () => {
cy.location('hash').should('equal', '#/specs')
cy.contains('.router-link-exact-active', 'Specs')

cy.findAllByTestId('spec-item').first().click()
cy.location('hash').should('contain', '#/specs/runner')
cy.contains('.router-link-exact-active', 'Specs').should('not.exist')
cy.percySnapshot()

cy.get('a[href="#/specs"]').click()
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good call!

Copy link
Member

Choose a reason for hiding this comment

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

The update looks elegant!

cy.location('hash').should('equal', '#/specs')
cy.contains('.router-link-exact-active', 'Specs')
})

it('has a menu item labeled "Settings" which takes you to the Settings page', () => {
cy.findByTestId('app-header-bar').findByText('Settings').should('not.exist')
cy.findByText('Settings').should('be.visible')
Expand Down
5 changes: 5 additions & 0 deletions packages/app/src/navigation/SidebarNavigation.cy.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,4 +55,9 @@ describe('SidebarNavigation', () => {
cy.findByTestId('keyboard-modal-trigger').focus().type('{enter}')
cy.findByTestId('keyboard-modal').should('be.visible')
})

it('uses exact match for router link active class', () => {
mountComponent()
cy.get('[href="#/specs"]').should('have.class', 'router-link-exact-active')
})
})
4 changes: 2 additions & 2 deletions packages/app/src/navigation/SidebarNavigation.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,12 @@
>
<RouterLink
v-for="item in navigation"
v-slot="{ isActive }"
v-slot="{ isExactActive }"
:key="item.name"
:to="item.href"
>
<SidebarNavigationRow
:active="isActive"
:active="isExactActive"
Comment on lines -46 to +47
Copy link
Contributor Author

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.

Copy link
Contributor

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.

:icon="item.icon"
:name="item.name"
:is-nav-bar-expanded="isNavBarExpanded"
Expand Down