-
Notifications
You must be signed in to change notification settings - Fork 16
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
2575: Focus trap bottom action sheet #2576
Conversation
5f069a5
to
36d41e3
Compare
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.
Tested on firefox
web/src/components/Modal.tsx
Outdated
@@ -62,9 +62,9 @@ const Modal = ({ title, closeModal, children, direction, wrapInPortal = false }: | |||
|
|||
return () => layoutElement?.setAttribute('aria-hidden', 'false') | |||
}, []) | |||
|
|||
// display check option is needed for portals - https://github.com/focus-trap/tabbable/blob/master/CHANGELOG.md#532 |
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 think you linked the wrong release, shouldnt it be https://github.com/focus-trap/tabbable/blob/master/CHANGELOG.md#600?
It says
restore old (incorrect) behavior
Should we investigate a better fix?
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.
Gonna adjust the link.
I just investigated some time and i didn't find an easy solution. The trap works well with this option, so no idea if its really worth to investigate more time @steffenkleinle
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.
Looks good.
Short description
Focus trap lib since
10.x
can not detect nodes from a portal (bottom action sheet) and will throw an error (opening feedback modal)Proposed changes
Side effects
Resolved issues
Fixes: #2575