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

set subnav contains current item to false #4722

Closed
wants to merge 2 commits into from
Closed

set subnav contains current item to false #4722

wants to merge 2 commits into from

Conversation

Hanskrogh
Copy link

Closes #4721

before-recording.mov
after-recording.mov

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

@Hanskrogh Hanskrogh requested a review from a team as a code owner July 7, 2024 08:11
@Hanskrogh Hanskrogh requested a review from TylerJDev July 7, 2024 08:11
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jul 7, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: de21ed4

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

@broccolinisoup broccolinisoup mentioned this pull request Jul 16, 2024
Open
@TylerJDev
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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

@Hanskrogh
Copy link
Author

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? :)

TylerJDev
TylerJDev reviewed Aug 20, 2024
View reviewed changes
Copy link
Contributor

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

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

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!

packages/react/src/NavList/NavList.test.tsx
Sub Item
</NavList.Item>
</NavList.SubNav>
<NavList>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see the <ThemeProvider> was removed here and was just curious if we removed it intentionally? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

@TylerJDev It was not intentionally ;)

Copy link
Author

Choose a reason for hiding this comment

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

@TylerJDev It seems like the npx changeset only allows me to create a Major release... I do not believe this is a major release :)

@github-actions GitHub Actions
Copy link
Contributor

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.

@github-actions github-actions bot added the Stale label Oct 21, 2024
@github-actions github-actions bot closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TylerJDev TylerJDev TylerJDev left review comments

Assignees
No one assigned
Labels
Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

containsCurrentItem State Not Reset in NavList Component.
2 participants
@Hanskrogh @TylerJDev