-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Popover] Add ariaHaspopup #2248
Conversation
4f0a743
to
b969dcc
Compare
@@ -0,0 +1,23 @@ | |||
import {AriaAttributes} from 'react'; |
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.
It seems this chunk of code was never tested. When I was trying to add a few tests I kept getting stale dom nodes. I broke this utility into its own module so we can easily mock it in Popover
.
@@ -1,9 +1,33 @@ | |||
import React from 'react'; | |||
import {mountWithAppProvider, findByTestID} from 'test-utilities/legacy'; | |||
import {Popover} from '../Popover'; | |||
import * as SetActivatorAttributes from '../set-activator-attributes'; |
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.
Importing like this to spyon the module
de8e142
to
e217898
Compare
@@ -415,6 +417,8 @@ See Apple’s Human Interface Guidelines and API documentation about accessibili | |||
|
|||
Popovers usually contain an [option list](https://polaris.shopify.com/components/lists-and-tables/option-list) or an [action list](https://polaris.shopify.com/components/actions/action-list), but can also contain other controls or content. | |||
|
|||
A popover can contain many numerous types of content. Whether it's a menu, grid or something entirely different! When `aria-expanded` is applied to an element, `aria-haspopup` will default to `menu`. To assist screen readers you'll need to pass `ariaHaspopup` to `Popover`. |
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.
A few thoughts here:
- I'd like to remove the "Whether it's..." fragment, since it doesn't reference other patterns or components.
- In reference to
aria-expanded
andaria-haspopup
, it's a bit more complicated. I think we could simplify this by saying something like, "TheariaHaspopup
prop addsaria-haspopup="true"
to the button. This should only be used when the popover contains a complex feature like an ARIA menu, grid, listbox, or similar features." - We typically want to use people-centered language, so we'd want to say "to assist people who use screen readers" instead of "to assist screen readers".
Let's chat about this one on Tuesday, and involve @sadiesaurus if she's available.
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.
Hi @sadiesaurus , what are your thoughts on this?
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.
Heya Andrew! I agree with Devon's points. I'll make a suggestion for this paragraph which you can feel free to edit.
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.
Here's my stab at a rewrite! Though, I'm not certain if I've accurately captured the context, so let me know if something is wrong or missing.
A popover can contain many numerous types of content. Whether it's a menu, grid or something entirely different! When `aria-expanded` is applied to an element, `aria-haspopup` will default to `menu`. To assist screen readers you'll need to pass `ariaHaspopup` to `Popover`. | |
Popovers can contain many types of content. If a popover contains a complex feature such as an ARIA menu, grid, listbox, or similar feature, then use the `ariaHaspopup` prop. This adds `aria-haspopup="true"` to the button, which is helpful information for people who use screen readers. |
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.
It looks great! I made one small adjustment since you can provide more values than "true" to ariaHasPopup
(boolean | 'false' | 'true' | 'menu' | 'listbox' | 'tree' | 'grid' | 'dialog'
)
e217898
to
e155215
Compare
cc/ @dpersing |
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.
One comment but otherwise looks good to me
src/components/Popover/README.md
Outdated
@@ -415,6 +417,10 @@ See Apple’s Human Interface Guidelines and API documentation about accessibili | |||
|
|||
Popovers usually contain an [option list](https://polaris.shopify.com/components/lists-and-tables/option-list) or an [action list](https://polaris.shopify.com/components/actions/action-list), but can also contain other controls or content. | |||
|
|||
A popover can contain many numerous types of content. Whether it's a menu, grid or something entirely different! When `aria-expanded` is applied to an element, `aria-haspopup` will default to `menu`. To assist screen readers you'll need to pass `ariaHaspopup` to `Popover`. | |||
|
|||
Popovers can contain many types of content. If a popover contains a complex feature such as an ARIA menu, grid, listbox, or similar feature, then use the `ariaHaspopup` prop. This adds `aria-haspopup` to the button, which is helpful information for people who use screen readers. |
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.
Was this section at line 422 meant to be removed?
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.
It was a bad rebase, thanks for catching this 🙏
9cf6d12
to
8366d57
Compare
8366d57
to
aac6d1f
Compare
WHY are these changes introduced?
Fixes #2187
WHAT is this pull request doing?
How to 🎩
Copy-paste this code in
playground/Playground.tsx
:🎩 checklist
README.md
with documentation changes