set subnav contains current item to false

[Hanskrogh]

Closes #4721

https://github.com/primer/react/assets/43749842/349ea2a5-bd23-407b-ba39-5e2640fab37c

https://github.com/primer/react/assets/43749842/0a7851da-a0fa-4091-adbd-1ee8d1eb8690

Changelog

New

Changed

1. Changed behavior of ItemWithSubNav within the NavList component.

Removed

Rollout strategy

• [ x] Patch release
• [ ] Minor release
• [ ] Major release; if selected, include a written rollout or migration plan
• [ ] None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

• [ ] Added/updated tests
• [ ] Added/updated documentation
• [ ] Added/updated previews (Storybook)
• [ ] Changes are SSR compatible
• [x] Tested in Chrome
• [x] Tested in Firefox
• [x] Tested in Safari
• [x] Tested in Edge
• [ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: de21ed496c9cc970cff7b1ba4e211bb80d534273

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[TylerJDev]

Thank you for the PR @Hanskrogh! This looks great ✨! I'm wondering, would it be possible for us to add a test in NavList.test.tsx so that we ensure that we always set containsCurrentItem to false when it is no longer the current item?

[Hanskrogh]

Thank you for the PR @Hanskrogh! This looks great ✨! I'm wondering, would it be possible for us to add a test in NavList.test.tsx so that we ensure that we always set containsCurrentItem to false when it is no longer the current item?

Hey @TylerJDev - I’ve added a snapshot test. It needed to be a snapshot test because the state only affects the ::after pseudo-element. I had to capture the initial styles and perform a couple of rerenders to ensure the component’s styles return to the initial state. 😊

Feel free to adjust as needed!

[TylerJDev]

Thanks a ton @Hanskrogh! I'll take a look at the changes before the week is up! Thank you again for the PR! ✨

[Hanskrogh]

Thanks a ton @Hanskrogh! I'll take a look at the changes before the week is up! Thank you again for the PR! ✨

Any news? :)

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[Hanskrogh]

I'm sorry for the delay @Hanskrogh! I'm coming at this a bit late, and now I'm having trouble reproducing this. Is/was there a page or example you tested this on? I followed the steps in #4721 but I suspect I'm missing a step.

Other than that and the comment I left, I think the only thing this PR would need is a changeset!

Hi there,

sorry for the late answer, you can actually kinda see the issue here: https://primer.style/product/getting-started/react/core-concepts/ (you can see parent NavList.Item is flickering everytime you change a route.)

the only reason why it is working on the documentation page, is because the sidebar is re-rendered on every navigation, i am not sure if this is done on purpose to fix the issue?