Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow popovers to be closed via the Escape key #502

Merged
merged 20 commits into from
Jun 11, 2020
Merged

Conversation

alexpaxton
Copy link
Contributor

@alexpaxton alexpaxton commented May 5, 2020

Closes #501

Changes

  • Popovers can be dismissed by hitting the escape key
    • The dialog component can be focusable and receive keyboard events
    • Keyboard events bubble up to the portal containing the popover and then it closes
    • PopoverDialog will make itself the browser's active element if nothing else is in focus
  • Update documentation
    • Explain nuances of prop combinations
    • Add more permutations of props for manual testing
  • Round numbers down to whole numbers when calculating popover position
    • This prevents fuzzy sub-pixel rendering of popovers
  • If visible is true it will supersede the escape key behavior in the interest of complete external control

Screenshots

Screen Shot 2020-05-21 at 12 07 48 PM

Example of PopoverDialog deferring focus to an input with autofocus={true}:
popover-doesnt-pull-focus

Checklist

Check all that apply

  • Updated documentation to reflect changes
  • Added entry to top of Changelog with link to PR (not issue)
  • Tests pass
  • Peer reviewed and approved
  • Signed CLA (if not already signed)

@alexpaxton alexpaxton changed the title Feat escape key Allow popovers to be closed via the Escape key May 5, 2020
@alexpaxton alexpaxton requested a review from mavarius May 5, 2020 21:06
Copy link
Collaborator

@mavarius mavarius left a comment

Choose a reason for hiding this comment

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

Two main issues:

  • The visibility prop can be used to make this a controlled or uncontrolled component
  • The keyboard listener should be specific to the popover that initiated it

@alexpaxton
Copy link
Contributor Author

@mavarius I'm gonna noodle on this more and see if I can find a way to only close a single popover at a time using the escape key

// in order to enable escape key behavior
useEffect(() => {
if (dialogRef.current) {
dialogRef.current.focus()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this inadvertently pull focus from an element within the dialog, like a button or input field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question... I'll try building an example of this in storybook

@alexpaxton
Copy link
Contributor Author

I updated PopoverDialog to only pull focus if no other element is in focus, which you can apparently check using document.activeElement. Also built a story that verifies popover focusing works with and without children using autofocus

Copy link
Collaborator

@mavarius mavarius left a comment

Choose a reason for hiding this comment

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

Looks good! Ship it

@alexpaxton alexpaxton merged commit 3e17379 into master Jun 11, 2020
@alexpaxton alexpaxton deleted the feat-escape-key branch June 11, 2020 21:23
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add keyboard interaction to close popover
2 participants