feat(nav): Open secondary navigation in overlay when collapsed #92055
Conversation
github-actions
bot
added
the
Scope: Frontend
malwilley
force-pushed
the
malwilley/feat/nav-hover-section
branch
from
b98c3ad to
6dd7f30
Compare
| /** | ||
| * Whether to animate the hovercard in/out | ||
| */ | ||
| animated?: boolean; |
There was a problem hiding this comment.
Added this config option to allow the animation to be disabled
| update: (() => Promise<Partial<State>>) | null; | ||
| } | ||
|
|
||
| const HovercardContext = createContext<HovercardProviderValue>({ |
There was a problem hiding this comment.
Added a context here so that children can call reset to close the overlay when needed. This allows me to dismiss the overlay when a link is clicked.
| @@ -26,7 +28,7 @@ function CodecovSecondaryNav() { | |||
| }); | |||
|
|
|||
| return ( | |||
| <SecondaryNav> | |||
| <Fragment> | |||
There was a problem hiding this comment.
Moved this SecondaryNav wrapper from the *SecondaryNav components to a single parent component since we do not want it in the overlay.
There was a problem hiding this comment.
code looks good to me! minor nit: the overscroll behavior is a little wonky here
Screen.Recording.2025-05-21.at.1.35.34.PM.mov
also not sure if this is a problem, but the vercel preview doesn't work for me (i get the "It looks like you've hit an issue in our client application" message), but it works fine in local dev-ui
nvm working now
|
I tried this out locally and wanted to note that it’s not possible to get into the secondary sidebar when doing keyboard navigation: secondary-sidebar-keyboard.mp4Also, when trying to get into it with the mouse, I managed to get it to close before moving the mouse into it a bunch of times, which was a bit frustrating: secondary-navigation-mouse.mp4 |
@TkDodo thank you for pointing this out! I agree that ideally this would be keyboard navigable. I just spent some time trying to figure out how to make it so, but I'm not so sure I've found anything that feels right. There are a couple options:
Do you have any other ideas how to make these items keyboard-accessible? I might be missing something. As an aside, I was looking for comps in other products to see what they did. The ones I checked (Datadog and Amplitude) unfortunately are not keyboard accessible in their collapsed state 😞)
Ah yes agree that's the the best behavior. Removed the offset so there is no room for the menu to be dismissed in between the trigger and overlay. |
… better scroll behavior but remove elements from tab order
|
@TkDodo I'm going to merge this because I want to get some feedback on the UX before we start pushing to GA soon, but the keyboard accessibility is still on my mind so feel free to leave more comments or reach out if you have suggestions |
|
@malwilley have a look at how https://react-spectrum.adobe.com/react-aria/Menu.html#submenus It has keyboard navigation, fault tolerance and other goodies built in: https://react-spectrum.adobe.com/blog/creating-a-pointer-friendly-submenu-experience.html since we already use react-aria heavily, I think our menus should be built on this. |
Before, when the navigation was collapsed, we would pop open the sidebar when hovering anywhere in the primary nav. New behavior will display each individual section in an overlay.
CleanShot.2025-05-21.at.12.42.32.mp4