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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add options to set initial focus within modal #476

Merged
merged 5 commits into from Jun 1, 2021

Conversation

megawebmaster
Copy link
Contributor

Hi!

First of all: I really like your library - it's lean and works great. As I use it extensively I needed to work around issue with trapping focus within the modal, but without focusing first focusable element (like a button or link), but instead focus the modal container. This allows me to style :focus properly and have accessibility (so that keyboard users can browse the page properly).

To do this I decided to add options for FocusTrap that allows me to set where I want to have focus when trapped. I designed it in a way that allows people to update (it shouldn't change default behavior).

Let me know your thoughts! 馃挭

@codecov
Copy link

codecov bot commented May 12, 2021

Codecov Report

Merging #476 (02cbb04) into master (23cccc6) will decrease coverage by 1.96%.
The diff coverage is 95.61%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #476      +/-   ##
==========================================
- Coverage   97.20%   95.23%   -1.97%     
==========================================
  Files           6        6              
  Lines         179      189      +10     
  Branches       66       70       +4     
==========================================
+ Hits          174      180       +6     
- Misses          5        9       +4     
Impacted Files Coverage 螖
react-responsive-modal/src/FocusTrap.tsx 70.00% <50.00%> (-9.17%) 猬囷笍
react-responsive-modal/src/index.tsx 100.00% <100.00%> (酶)

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 23cccc6...02cbb04. Read the comment docs.

@megawebmaster megawebmaster force-pushed the master branch 3 times, most recently from 80a1888 to 6578ac4 Compare May 20, 2021 10:26
@megawebmaster
Copy link
Contributor Author

megawebmaster commented May 20, 2021

The code fails to build due to a weird error: offsetParent is null on all DOM nodes causing FocusTrap to think that nothing is visible and skipping the focus on a first visible element part. I filed a bug in @testing-library/react to get this fixed: testing-library/react-testing-library#915

EDIT: The jsdom does not support such case, so I decided to add a Cypress test and a proper docs page for the case of focusing modal. Works as expected 馃挭

react-responsive-modal/package.json Outdated Show resolved Hide resolved
react-responsive-modal/package.json Outdated Show resolved Hide resolved
react-responsive-modal/src/FocusTrap.tsx Outdated Show resolved Hide resolved
@pradel
Copy link
Owner

pradel commented May 27, 2021

In any case thanks for the amazing pr, looking forward to add this feature :)

@pradel
Copy link
Owner

pradel commented May 30, 2021

@megawebmaster do you think of a way to update the unit tests so the coverage is not decreasing?

@megawebmaster
Copy link
Contributor Author

megawebmaster commented May 31, 2021

@pradel - unfortunately this is impossible due to missing capabilities in JSDOM used as the environment by React Testing Library. I even tried to find solutions upstream (because I thought it's important to have a good coverage).

EDIT: Do you have any idea why size checking fails even though it passes locally with npx size-limit?

@pradel
Copy link
Owner

pradel commented Jun 1, 2021

Error creating comment. This can happen for PR's originating from a fork without write permissions.

This is the error message I can see for the size limit so all good :D

@pradel pradel changed the title Add options to trapping focus within modal feat: add options to set initial focus ref within modal Jun 1, 2021
@pradel pradel changed the title feat: add options to set initial focus ref within modal feat: add options to set initial focus within modal Jun 1, 2021
@pradel pradel merged commit 5bdc362 into pradel:master Jun 1, 2021
@pradel
Copy link
Owner

pradel commented Jun 1, 2021

@megawebmaster thank you so much for this amazing pr :D I will create a new release shortly

@megawebmaster
Copy link
Contributor Author

Great, thanks for the review and many good ideas how to make it even better 馃挭

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.

None yet

2 participants