Skip to content
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

[a11y] Popover accessibility docs #1408

Merged
merged 6 commits into from
May 7, 2019
Merged

[a11y] Popover accessibility docs #1408

merged 6 commits into from
May 7, 2019

Conversation

dpersing
Copy link
Contributor

@dpersing dpersing commented May 3, 2019

WHY are these changes introduced?

Adds accessibility guidance for the popover component, to appear in polaris-react docs and in the style guide.

See the draft Google doc for editing history for these changes and updates for other components and pages.

WHAT is this pull request doing?

  • Adds accessibility documentation for the popover component (web, iOS, and Android)
  • Adds an entry to UNRELEASED.md

How to 🎩

  1. Check out master from polaris-styleguide
  2. In another Terminal tab or window, check out this branch from polaris-react and run the instructions for testing in a consuming project.
  3. In the polaris-styleguide tab, run dev up && dev start.
  4. View changes after examples and props: https://polaris.myshopify.io/components/overlays/popover (web, iOS, and Android)

Screenshots

Web

Screenshot of the new content for web

Android

Screenshot of the new content for Android

iOS

Screenshot of the new content for iOS

@BPScott BPScott temporarily deployed to polaris-react-pr-1408 May 3, 2019 20:53 Inactive
@BPScott BPScott temporarily deployed to polaris-react-pr-1408 May 3, 2019 20:55 Inactive
@dpersing dpersing requested a review from sadiesaurus May 3, 2019 20:56
@dpersing dpersing changed the title Popover a11y docs [a11y] Popover accessibility docs May 3, 2019
@dpersing
Copy link
Contributor Author

dpersing commented May 7, 2019

@sadiesaurus Let me know if you have a chance to look at this! Popover came up today on a question from the inventory team, so I'd love to get this in our next release.

@BPScott BPScott temporarily deployed to polaris-react-pr-1408 May 7, 2019 19:46 Inactive
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just had a couple take 'em or leave 'em suggestions/questions 😀


### Keyboard support

- When a popover opens, focus moves to the popover container
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we mention the preventAutoFocus prop at all? I'm thinking there's opportunity here to make it clear when it's okay to use, like when autoFocus would prevent merchants from completing the action that triggers the popover. For example, when a text field listening for certain characters is the activator and autofocusing focusable items inside the activated popover would remove focus from the text field while typing; additional focus management needs to be implemented in this case.

(Click to view gif of example case described above)

2019-05-07 16 32 42

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came up in discussion about popovers today too, actually. Based on that and other questions about keyboard accessibility, I think I want to chunk out a separate page (probably under patterns and guides?) about keyboard interactions in general. Would love your thoughts on doing it that way, @chloe!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a great idea, as it comes up in a lot of our components 👍

Co-Authored-By: dpersing <devon.persing@shopify.com>
@BPScott BPScott temporarily deployed to polaris-react-pr-1408 May 7, 2019 21:08 Inactive
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

Successfully merging this pull request may close these issues.

3 participants