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

Components: Close popover by escape keypress #2697

Merged
merged 4 commits into from Sep 25, 2017

Conversation

Projects
None yet
1 participant
@aduth
Member

aduth commented Sep 7, 2017

Related: #2323

This pull request seeks to enhance the Popover component to handle escape keypress and focus return for closing the focused dialog. This also helps consolidate existing but inconsistently-applied approaches for this behavior.

Testing instructions:

Verify that a focused popover can be closed by clicking outside or pressing escape. When pressing escape, observe that focus returns to the element which had been focused prior to opening the dialog.

  1. Navigate to Gutenberg > New Post
  2. Click Insert in the header menu
  3. Press Escape
  4. Note that the Insert button is focused

(Repeat for Visibility, Schedule popovers)

Caveats:

When tabbing through to the end of a popover's tabbable elements, we should do one or the other of:

  • Circle back to beginning of tabbables in popover (constrain focus, as in a modal)
  • Direct focus to next in sequence from where popover had been mounted

aduth added some commits Sep 7, 2017

Sidebar: Close schedule by Popover prop
Click outside is handled internally by the Popover component
Components: Split popover onClose and onClickOutside
onClickOutside passes event which is relied upon by PostVisibility, but we don't want to guarantee this with signature of onClose
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 7, 2017

Codecov Report

Merging #2697 into master will increase coverage by 0.02%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2697      +/-   ##
==========================================
+ Coverage   33.64%   33.67%   +0.02%     
==========================================
  Files         185      185              
  Lines        5584     5591       +7     
  Branches      973      975       +2     
==========================================
+ Hits         1879     1883       +4     
- Misses       3138     3139       +1     
- Partials      567      569       +2
Impacted Files Coverage Δ
components/popover/detect-outside.js 25% <ø> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/inserter/menu.js 49.59% <100%> (+1.18%) ⬆️
editor/sidebar/post-schedule/index.js 66.66% <100%> (+1.66%) ⬆️
editor/sidebar/post-visibility/index.js 45.23% <25%> (-0.92%) ⬇️
components/popover/index.js 87.64% <37.5%> (-5.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6ceb8b...9c539d0. Read the comment docs.

codecov bot commented Sep 7, 2017

Codecov Report

Merging #2697 into master will increase coverage by 0.02%.
The diff coverage is 41.17%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2697      +/-   ##
==========================================
+ Coverage   33.64%   33.67%   +0.02%     
==========================================
  Files         185      185              
  Lines        5584     5591       +7     
  Branches      973      975       +2     
==========================================
+ Hits         1879     1883       +4     
- Misses       3138     3139       +1     
- Partials      567      569       +2
Impacted Files Coverage Δ
components/popover/detect-outside.js 25% <ø> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/inserter/menu.js 49.59% <100%> (+1.18%) ⬆️
editor/sidebar/post-schedule/index.js 66.66% <100%> (+1.66%) ⬆️
editor/sidebar/post-visibility/index.js 45.23% <25%> (-0.92%) ⬇️
components/popover/index.js 87.64% <37.5%> (-5.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6ceb8b...9c539d0. Read the comment docs.

@aduth aduth merged commit 75a5c16 into master Sep 25, 2017

3 checks passed

codecov/project 33.67% (+0.02%) compared to c6ceb8b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the update/popover-close-on-esc branch Sep 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment