-
Notifications
You must be signed in to change notification settings - Fork 526
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
[UnderlineNav] : Keep selected item always visible #2361
Conversation
🦋 Changeset detectedLatest commit: 043dbe3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
5c3d759
to
d9a1135
Compare
bc71984
to
c75ef96
Compare
665853e
to
f29376d
Compare
f29376d
to
4d88285
Compare
Hi @danielguillan 👋🏼 Keeping the selected item visible is ready for you to review 😊 This PR includes both overflow behaviour as well as the loading counter states. Feel free to review and test them as well here. |
@@ -106,14 +105,19 @@ const overflowEffect = ( | |||
for (const [index, child] of childArray.entries()) { | |||
if (index < numberOfItemsPossibleWithMoreMenu) { | |||
items.push(child) | |||
// keeping selected item always visible. | |||
} else if (child.props.selected) { |
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.
yay ssr friendly hack 👍
This looks great! My only feedback is that we should persist the item order when selecting visible items. That is, when we swap a selected item to make it visible and the user then selects an already visible item, we should keep the current (updated) item order instead of restoring the original one. This is to prevent the visual change that happens away from the user's cursor which can be confusing. Always.visible.item.swap.mp4In the recording above, when we click "Actions", the "Insights" item shouldn't be swapped back with "Projects". "Insights" should still be the last item. Same for when we click "Pull requests", the "Settings" item shouldn't be swapped back with "Projects". |
👋🏼 @danielguillan Thanks so much for your review! No worries, I wasn't sure about that ordering behaviour and it makes sense to reduce the layout shifts. I'll revert it now |
7615b86
to
246df5a
Compare
751b761
to
0cf2156
Compare
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.
Lets go!
Closes #1296
Storybook link: https://primer-1475f2ddc1-13348165.drafts.github.io/storybook/?path=/story/components-underlinenav
This PR is for keeping UnderlineNav's selected link always visible. This means;
Screen Recordings
Screen.Recording.2022-09-30.at.10.44.04.pm.mov
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.