Skip to content
This repository has been archived by the owner on Oct 6, 2020. It is now read-only.

fix(Modal): Lock focus #41

Merged
merged 7 commits into from
May 2, 2019
Merged

fix(Modal): Lock focus #41

merged 7 commits into from
May 2, 2019

Conversation

cehsu
Copy link
Contributor

@cehsu cehsu commented May 1, 2019

These changes:

  • Lock focus in modals (via react-focus-lock)
  • Adds an example with a dropdown (to illustrate how returnFocus is handled)

image

Next steps:

  • add VRT with puppeteer, and tie into CI
  • consider using react-focus-on for modals as recommended by react-focus-lock
  • close on escape keypress (Firefox)

@cehsu cehsu requested a review from a team May 1, 2019 18:12
@codecov
Copy link

codecov bot commented May 1, 2019

Codecov Report

Merging #41 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #41   +/-   ##
=======================================
  Coverage   68.36%   68.36%           
=======================================
  Files          22       22           
  Lines         433      433           
  Branches       91       91           
=======================================
  Hits          296      296           
  Misses        107      107           
  Partials       30       30

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 b2758f5...1913100. Read the comment docs.

Copy link
Contributor

@kylealwyn kylealwyn left a comment

Choose a reason for hiding this comment

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

This looks lovely!

Also, not sure if introduced here (probably not) but noticed in the long content example that the top of the modal is cut off and can't scroll up to see it.

Screen Shot 2019-05-01 at 11 26 26 AM

src/Modal/Modal.js Outdated Show resolved Hide resolved
@cehsu
Copy link
Contributor Author

cehsu commented May 1, 2019

@kylealwyn the scrolling issue is indeed new, good catch, investigating fixed.

image

@cehsu cehsu requested a review from a team May 1, 2019 21:37
Copy link
Contributor

@kylealwyn kylealwyn left a comment

Choose a reason for hiding this comment

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

🎉

src/Modal/Modal.mdx Outdated Show resolved Hide resolved
src/Modal/Modal.mdx Outdated Show resolved Hide resolved
@cehsu cehsu merged commit 9e7b7d3 into master May 2, 2019
@cehsu cehsu deleted the fix/modal-focus branch May 2, 2019 18:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants