-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Update useModalOverlay types #5116
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
Conversation
Add extra types from useOverlay props
|
Sorry for the close/re-open spam - was trying to shake the CLABot out of its slumber 😆 |
|
I've actually had to do something similar when implementing SubMenus for |
|
Right on, thanks. In case it's useful, the specific use case here came up when with our design system's Dialog component. When a Dialog contained a legacy Select component, attempting to select an entry from the dropdown would close the dialog.
|
| */ | ||
| isKeyboardDismissDisabled?: boolean | ||
| } | ||
| export interface AriaModalOverlayProps extends AriaOverlayProps {} |
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.
We discussed adding shouldCloseOnInteractOutside to usePopover for submenus today, leaning towards being fine adding it there and for useModalOverlay as well as you've done here. I'd personally would prefer just adding shouldCloseOnInteractOutside only here instead of all the overlay props until we get use cases for the rest of those props but happy for the rest of the team to weigh in.
Would you mind adding a basic test for this? You can probably add a new test file for useModalOverlay or for the RAC Modal since it extends from AriaModalOverlayProps. https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/overlays/test/useModal.test.js or any of the tests in https://github.com/adobe/react-spectrum/tree/main/packages/react-aria-components/test could serve as a guide
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.
Thanks for the review and sorry for the delay, we had an onsite at work which naturally took all my attention last week.
Good to hear - I'll look to hack together a test this week.
8cb1a5d to
3013156
Compare
a79adcf to
3013156
Compare
|
This is now supported via #5986. |
Extended the type of the
useModalOverlay()props fromuseOverlay()'s props.I want to be able to pass
shouldCloseOnInteractOutsidetouseModalOverlayso that I can prevent closing in certain cases.The values are already being passed through to
useOverlay()so I went ahead and simply extendedAriaOverlayPropsinstead of adding more and more repeating variables. I'm not sure if there's a particular reason that approach wasn't taken already, so feel free to recommend the alternative.It seems like any docs for this should be updated automatically but lemme know if I'm missing anything here.
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: