-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(exoflex): support a11y for menu item #457
Conversation
@@ -2,6 +2,7 @@ import React from 'react'; | |||
import { Menu as PaperMenu } from 'react-native-paper'; | |||
import Text from './Text'; | |||
import useTheme from '../helpers/useTheme'; | |||
import { AccessibilityProps, View } from 'react-native'; | |||
|
|||
export type MenuProps = OmitPaperTheme<typeof PaperMenu>; |
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.
What about accessibility for Menu?
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 skipped it as we can't access inside Paper's Menu. But should we just wrap it in a View and put the a11y props into that View similar to what I did with the Menu Item?
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.
Yes, let's wrap the Menu inside a View, similar to Menu Item.
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, it kinda feels off by doing that as the component that we wrapped turns out to be the anchor
instead of the parent of Menu.Item
.
Here is the wrapper that we give a role as menu
.
From the image above, the wrapper (menupopup) is only wrapping the anchor
which is a button. That's why it doesn't show any menu item from the tree.
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.
I see. In that case, I'm indifferent on whether we add the menu
accessibility at all.
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.
Alright then, I think we could just skip it until Paper implement it or we fork Paper and implement it from there.
Add a11y support for native and web Menu Item component.