[NavigationMenu] add delay duration skip delay duration props #1478
Conversation
|
Not sure what's going on here, but it seems the CI checks haven't run and I don't have the usual button to allow them to run. ✌️ |
|
Here's to hoping you can run the workflows now. Let me know if something is wrong. I just want to take a moment to say that I love this project and the work you guys are doing. Would definitely try to contribute more if my college workload wasn't killer. ❤️ |
| @@ -28,6 +28,7 @@ type Direction = 'ltr' | 'rtl'; | |||
| * -----------------------------------------------------------------------------------------------*/ | |||
|
|
|||
| const NAVIGATION_MENU_NAME = 'NavigationMenu'; | |||
| const DEFAULT_DELAY_DURATION = 700; | |||
There was a problem hiding this comment.
Whilst 700 feels right to me for Tooltip, it feels too long for NavigationMenu in my opinion.
How do you feel about this @andy-hook?
There was a problem hiding this comment.
I agree, it definitely feels too long here. It may take some testing to get a good "feel" but something around the 100 mark could be a place to start. If we're too slow here it will exacerbate the issue of users thinking it's a click interaction and immediately closing the menu.
There was a problem hiding this comment.
If we're too slow here it will exacerbate the issue of users thinking it's a click interaction and immediately closing the menu.
Yes, that's exactly what I thought too.
| @@ -0,0 +1,5 @@ | |||
| releases: | |||
| "@radix-ui/react-navigation-menu": patch | |||
There was a problem hiding this comment.
This should probably be tagged as a minor because it introduces a new feature, but also because the out of the box behaviour is "slightly" altered (slight delay by default).
| delayDuration = DEFAULT_DELAY_DURATION, | ||
| skipDelayDuration = 300, |
There was a problem hiding this comment.
I think I'd prefer seeing the defaults being applied at the props level in the components, rather than on the provider.
I know this means multiple places but I think it's better that way so we can always assume a value internally anywhere.
| @@ -208,9 +223,16 @@ const NavigationMenuProvider: React.FC<NavigationMenuProviderProps> = ( | |||
| onChange: onValueChange, | |||
| defaultProp: defaultValue, | |||
| }); | |||
| const [shouldSkipDelay, setShouldSkipDelay] = React.useState(false); | |||
There was a problem hiding this comment.
It would be good if we could follow what was done in Tooltip for consistency purpose.
We track rather whether open should be delayed or not with then a separate handleOpen and handleDelayedOpen.
If we could stay as close as possible to this implementation that would be great 🙏
|
I've addressed this feedback plus other issues I noticed via #1716 so will close this in favour. Thanks again for your efforts and contributions to Radix @innub , much appreciated. |
Closes #1457