Skip to content

Navigation hook in react-aria using Disclosure pattern #3817

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

Closed
vbudovski opened this issue Dec 6, 2022 · 9 comments
Closed

Navigation hook in react-aria using Disclosure pattern #3817

vbudovski opened this issue Dec 6, 2022 · 9 comments

Comments

@vbudovski
Copy link

vbudovski commented Dec 6, 2022

Is there any interest in a navigation component in react-aria utilising the disclosure pattern?

🙋 Feature Request

A navigation is a common component in web apps, and none of the existing hooks meet the specific requirement. useMenu might be the closest, but isn't quite right.

My proposed implementation would consist of 2 hooks: userNavigation and useNavigationItem and utilise TreeState from react-stately. Initial attempt at implementing https://codesandbox.io/s/cranky-field-u85oox

💻 Examples

https://www.w3.org/WAI/ARIA/apg/example-index/disclosure/disclosure-navigation.html

@LFDanLu
Copy link
Member

LFDanLu commented Dec 6, 2022

Interesting, that feels like a common enough pattern to exist in our Spectrum design system (and therefore would have a good chance having an aria hook made for it) but I'm having a hard time finding it. From a quick glance, this experience looks like a list/collection of menu triggers where list navigation could be handled via extending the existing list layout/keyboard delegate code and reusing some of our list hooks (albeit with some modifications for horizontal navigation rather than vertical). This seems roughly in line with what you've implemented, but I'd have to do some additional research to see if there is anything else needed.

@reidbarber
Copy link
Member

I also wonder if a useMenuBar hook would cover this use case, where the children use both useMenuItem and useMenuTrigger. And not seeing much about menus on Spectrum Headers page.

@nicksergeant
Copy link

@vbudovski we ran into a need for RAC navigation as well and began implementing your solution here (which is an awesome start!). Fairly new to RAC and discovered that implementation at https://codesandbox.io/s/cranky-field-u85oox is not handling click/touch events in Safari. Trying to debug and figure out why that might be, but curious if you ever put this into production and discovered a similar thing / fix for it?

@nicksergeant
Copy link

@vbudovski quick GIF demo screencast here - Safari on the left (no errors in console), Chrome on the right:

CleanShot 2024-02-10 at 21 22 11

@vbudovski
Copy link
Author

vbudovski commented Feb 11, 2024

@nicksergeant I can't say that I've tested it much, as this was more of a proof-of-concept. You can try to update the packages to the lastest version as this sandbox was written a year ago. Recent versions of React Aria now contain the Toolbar component. You could try to use that instead of this custom hook. Perhaps some accessibility experts can chime in on how accessible that approach is. Here's a sandbox using this new approach: https://codesandbox.io/p/sandbox/intelligent-darwin-snslfn?file=%2Fsrc%2FApp.tsx

@nicksergeant
Copy link

@vbudovski thanks for the reply! That approach is using the Menu component (role="menu" under the hood) which should be avoided for site navigations: https://adrianroselli.com/2017/10/dont-use-aria-menu-roles-for-site-nav.html

I'll keep tinkering with your example. It's very strange, no onClick handlers are firing anywhere in the stack of Navigation components in Safari, but Chrome / FF works just fine. My guess is this is related to the (apparently many) Safari / touch bugs in RAC: https://github.com/adobe/react-spectrum/issues?q=is%3Aissue+is%3Aopen+safari+touch

@aaronmars
Copy link

The above example from @vbudovski using the newer approach does not quite implement the disclosure navigation pattern, and it also suffers from this bug: #5338

@reidbarber
Copy link
Member

We're going to have useDisclosure and useDisclosureState hooks as a part of #6931

@snowystinger
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants