`<UnderlineNav>` menu now uses the Anchored Position on smaller screen sizes to not clip, or go out of bounds.
Closes #4059
Changelog
New
This PR fixes the issue with the <UnderlineNavMenu> on smaller screens using the menu. Where it would normally overflow to the left, this makes sure we align the absolute item using the AnchoredPosition API that you have provided.
Old Behavior:
New Behavior (321px >):
On very small screens (not supported, but hey - why not):
Changed
- Updates to
menuStylesin the<UnderlineNav>It has become more complex, as the style has become complex as well. I am forced to check now for how many pixels we have available in thelistRef. I am also using thegetAnchoredPositionAPI now to calculate the position of the element, but only for smaller screens, as to not mess with themin-widththat was previously set.
Removed
- Nothing has been removed.
Rollout strategy
- [ ] Patch release
- [x] 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
- [x] Added/updated tests
- [x] Added/updated documentation
- [x] Added/updated previews (Storybook)
- [x] 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 detected
Latest commit: bc5a69fef4b6b59e46086fb77dd8a6c8bee3dd8b
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @primer/react | Patch |
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
Sorry :pray: . I seem to have done a wonky-merge. Recreated the PR, feedback and conversation can be found here.
Copy+Pasting my last message:
Woop woop! 🎉
Thank you a billion for the feedback, @broccolinisoup. It really helps with the implementation and understanding what the aim is 🙏.
Here's how I suggest we fix this:
I kind of like the margin: 0 solution which forces the menu to the right. It is simple and works. However, when the container does not offer enough room (as the more button is left-aligned), I suggest we use getAnchoredPosition like you suggested.
I've updated the code to do the following:
If we do not have enough space:
We use getAnchoredPosition to position our elements. The only real combination I found that would work would be to combine {align: 'center', side: 'outside-left'}. But if I understand the [documentation correctly](https://github.com/primer/behaviors/blob/main/docs/anchored-position.md#positionsettings-interface), that should be the right setting for popovers/menus.
If we have enough space:
Keep using right: 0 to make it right-aligned.
Again, thanks for the feedback! If I missed something, or, misunderstood something - please let me know and ill get to it 🙏
To summarize the current fix, based on @broccolinisoup's feedback:
- I moved the clientWidth calculation outside of the menuStyles to not force the getAnchoredPosition to run on each render, instead being handled as a ternary when setting the
sxprop. - Created a constant for the min-width called baseMenuMinWidth. The baseMenuMinWidth is now referenced as the value we check, use, or test the 192px min-width. That way, we only use a single value.
- Created a simple constant for the base menuStyles called baseMenuStyles, which contains right: '0'.
- Simplified the test case as the menuStyles is now simpler.
- The getAnchoredPosition now uses {align: 'start', side: 'outside-bottom'} for placement.
Again, thank you for a clear response and great feedback. This has been fun working on (and I hope it did not cause too much overhead for you), and if there's something missing - let me know!
@joshblack I was testing https://github.com/primer/react/pull/4235, it cancels the deploy preview (fork) action in this PR. Do you happen to know why that is failing to run? Also what is the best way you recommend to debug cancelled actions? I usually have a hard time debugging them, well, because there is not much proper logs 🫠