UnderlineNav overflow behaviour implementation #2297
Conversation
🦋 Changeset detectedLatest commit: 80e52e3 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 📦
|
broccolinisoup
force-pushed
the
broccolinisoup/UnderlineNav-states
branch
from
d87eb23
to
fcf6cf5
Compare
broccolinisoup
force-pushed
the
broccolinisoup/underlineNav-overflow
branch
from
c3d6d67
to
fc79da0
Compare
broccolinisoup
force-pushed
the
broccolinisoup/underlineNav-overflow
branch
from
fc79da0
to
5c13fb5
Compare
| const {theme} = useTheme() | ||
| useLayoutEffect(() => { | ||
| const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect() | ||
| // might want to select this better | ||
| const icon = (ref as MutableRefObject<HTMLElement>).current.children[0].children[0] |
There was a problem hiding this comment.
maybe you can add data-component?
There was a problem hiding this comment.
I tried selecting the icons using data-component item.getAttribute('data-component') === 'leadingIcon' but icon gets undefined if any item goes into more menu and comes back 🤔 Weird, I'll look into it again but keeping as is for now.
broccolinisoup
force-pushed
the
broccolinisoup/underlineNav-overflow
branch
from
b3650f8
to
6231df7
Compare
|
Adding skip changeset label just to avoid running changeset while keeping the PR draft. I'll remove it and add changeset when it is ready for review. |
|
This is looking great @broccolinisoup! My only concern, as you've mentioned in the recording, is that the "more" menu behavior on coarse pointer devices is problematic on small viewports. I don't think it's a good pattern to solve this problem on mobile devices. I've added a comment on the design issue to see if we can find alternatives. |
Awesome! @danielguillan We have the scroll behaviour in the commit, so we can always bring it back. I am keeping an eye on the design issue to see what our next step is. |
|
@danielguillan I reverted the commit to bring the scroll behaviour back and addressed your feedback around it as well. The only thing I would say is that the slide-in and out animated effect is a bit tricky to do with react. I understand about giving us more real estate. I played with it a little bit more to give more space to click while maintaining the padding etc. Do you think we can go with the current implementation for now and merge this PR if all other concerns are address? It became quite big and I have another PR coming up (making selected item always visible) so would be great to merge this to avoid conflicts and make it available under the drafts? In the meantime, I'll try to get some support for css & react animation to achieve slide in and out behaviour. |
Co-authored-by: Cole Bemis <colebemis@github.com>
Closes #1251
This PR addresses the overflow behaviour implementation that is described in the design issue.
TODO:
Moremenu button stylesCurrent built on storybook: https://primer-463f4c80a3-13348165.drafts.github.io/storybook/?path=/story/layout-underlinenav-examples--internal-responsive-nav
Current built on docs: https://primer-463f4c80a3-13348165.drafts.github.io/drafts/UnderlineNav2
Light Theme
More Menu
Scroll
Dark Theme (Dimmed)
More Menu
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.