-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
feat: add options to set initial focus within modal #476
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
80a1888
to
6578ac4
Compare
The code fails to build due to a weird error: EDIT: The |
In any case thanks for the amazing pr, looking forward to add this feature :) |
@megawebmaster do you think of a way to update the unit tests so the coverage is not decreasing? |
@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 |
This is the error message I can see for the size limit so all good :D |
@megawebmaster thank you so much for this amazing pr :D I will create a new release shortly |
Great, thanks for the review and many good ideas how to make it even better 💪 |
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! 💪