-
Notifications
You must be signed in to change notification settings - Fork 27
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
Using Safe triangle to improve submenu navigation #3536
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: ac8c35c The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
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 |
Thank you @ddouglasz for working on this ❤️ I've noticed in the screencast from the description that the safe triangle only changes when cursor moves within the menu item boundaries. If it goes beyond the menu item, the triangle is set for good. |
Thank you Kacper 🙏🏾 |
@kark thanks for your feedback again, after a couple of findings, I discovered a flaw with this implementation: Perhaps the current implementation that allows the the Would be nice to know what @FilPob also thinks about this 🙏🏾 |
Thanks! Perhaps we should consider adding a timeout for the triangle, similar to what we can see in your MacOS screencast. 🤔 |
Updated here: 25c5b47 |
This PR is missing a Jira ticket reference in the title or description. |
🥷 Code experts: emmenko emmenko has most 🧠 knowledge in the files. See details
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame:
Knowledge based on git-blame: To learn more about /:\ gitStream - Visit our Docs |
96b0a8e
to
dcc9928
Compare
0ed4d53
to
f05a8e3
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.
Thanks for being persistent and making it work 👏 .
I have only some suggestions, nothing that would block approval.
packages/application-shell/src/components/navbar/menu-items.styles.ts
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/navbar/menu-items.styles.ts
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/navbar/menu-items.tsx
Outdated
Show resolved
Hide resolved
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.
Thank you, Douglas! That's a great improvement 👏. I have to admit, I'm still trying to nail down where the mouse movement speed threshold is set so that the safe triangle is applied, but overall, this looks great!
packages/application-shell/src/components/navbar/menu-items.styles.ts
Outdated
Show resolved
Hide resolved
top: 0; | ||
bottom: 0; | ||
right: 100%; | ||
width: calc(100% - 16px); |
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.
I wonder, where does this 16px
subtraction come from?
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.
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.
Thanks got it 👍 Could we also add a comment here explaining that we account for the scrollable menu's padding and use the spacing30
design token?
packages/application-shell/src/components/navbar/use-navbar-state-manager.ts
Outdated
Show resolved
Hide resolved
packages/application-shell/src/components/navbar/use-navbar-state-manager.ts
Outdated
Show resolved
Hide resolved
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.
🙌
top: 0; | ||
bottom: 0; | ||
right: 100%; | ||
width: calc(100% - 16px); |
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.
Thanks got it 👍 Could we also add a comment here explaining that we account for the scrollable menu's padding and use the spacing30
design token?
@@ -256,6 +266,18 @@ const useNavbarStateManager = (props: HookProps) => { | |||
[state.activeItemIndex] | |||
); | |||
|
|||
const handleMouseMove = useCallback( |
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.
nit: just noticed that we have two functions named handleMouseMove
that do different things. Could we rename this one to be more descriptive?
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.
updated here: ac8c35c
const safeAreaWidth = submenuSafeAreaRefBoundingClientRect?.width || 0; | ||
const safeAreaHeight = submenuSafeAreaRefBoundingClientRect?.height || 0; | ||
|
||
const handleMouseMove = useCallback( |
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.
Actually, let's rename this one too. To e.g. calculateSafeAreaStartPositon
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.
updated here: ac8c35c
Thank you 🙌🏽
Summary
Using the Safe triangle pattern to improve submenu navigation
Description
Improve the submenu navigation by adding a safe area for users to smoothly navigate diagonally between the main menu and the submenu items, without awkwardly going out of focus.
Background:
https://x.com/shadeed9/status/1745707615517032449?t=XnNt9gNwPHLyfrMfHEnFuA&s=03
Before:
After (The visible triangle is only for the demo. It is not included in the PR):