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

Create hooks for Focus Trap and ESC listener and use in Modal, Popover and Tooltip #351

Merged
merged 17 commits into from
Jun 28, 2019

Conversation

lavelle
Copy link
Contributor

@lavelle lavelle commented Jun 26, 2019

Scope of this change:

  • Extract out a hook for the ESC handler
  • Extract out a hook for focus trapping
  • Use them in Popover, Tooltip and ModalCurtain
  • Convert ModalCurtain to a function component with hooks in order to allow it to use the new hooks
  • Extract out some Tooltip logic into a function component to use the hooks.

Not doing:

  • The main Tooltip is still a class – converting it to be a function component should be another PR, since it has a lot of lifecycle hooks and state, and we want to make sure we don't break anything. When we do this, Tooltip can use the hook directly.

  • Converting the deprecated ModalStandard and it's underlying ModalBase to use these hooks. These components have their own ESC handling, but it's not worth the effort to rewrite them as function components. We should work on removing uses in website and deleting them.

@lavelle lavelle changed the title Refactor esc listener into hook and use in modal and popover Create hooks for Focus Trap and ESC listener and use in Modal, Popover and Tooltip Jun 26, 2019
@danoc
Copy link
Member

danoc commented Jun 27, 2019

image

I'm getting this on Chrome when clicking on the button to open the popover.

Can you either:

  1. Remove the outline from the outer container? (Normally I'm against removing outlines, but the outermost element is not actually interactive and when the user hits Tab they'll see a focus state.)
  2. Work with Jon to restyle it?

I'd personally prefer the first option but am open the second if you feel strongly.

@lavelle
Copy link
Contributor Author

lavelle commented Jun 27, 2019

removed the outline

Copy link
Member

@danoc danoc 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! ✨

@lavelle lavelle merged commit 92839f8 into master Jun 28, 2019
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.

2 participants