-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Bugfix/tab focus #448
Bugfix/tab focus #448
Conversation
@helloanoop Can you review this and let me know if something needs to be added? |
This is a QOL improvement. Will review and merge this post our 1.0.0 launch on Oct 28th. |
@@ -78,10 +79,30 @@ const Modal = ({ | |||
}; | |||
|
|||
useEffect(() => { | |||
document.addEventListener('keydown', escFunction, false); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend abstracting this to a custom hook that handles focus trap
|
||
console.log(focusableElements); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove log
Hey @srikary12 ! Thanks for working on this. We made some updates on top of your PR and the changes have been merged in #3075 Closing this PR |
This PR attempts to implement Tab Order as discussed in #129
The Tab order stays inside Modal.
This is my first issue, please let me know if I miss something. 😄