Skip to content

[Dialog][AlertDialog][Popover][DropdownMenu][ContextMenu] Modality #700

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

Merged
merged 12 commits into from
Aug 2, 2021

Conversation

benoitgrelard
Copy link
Contributor

@benoitgrelard benoitgrelard commented Jun 9, 2021

Closes #585
Closes #566

This PR adds modality support to menus, dialogs and popovers. Originally we exposed a very granular api for managing pointer events, scroll locks and focus control. The most common use case for these props was to apply differing levels of modality depending on consumer needs.

By consolidating and streamlining these into a single modal prop we are able to reduce the api surface while making the outcome easier and faster to achieve. This change also allows us to provide consistent handling across components and improve some additional handling under the hood.

Example (DropdownMenu)

CleanShot.2021-07-13.at.15.00.23.mp4

@benoitgrelard benoitgrelard force-pushed the modality branch 2 times, most recently from 9e6ea31 to 41d2762 Compare June 23, 2021 20:11
@andy-hook andy-hook force-pushed the modality branch 2 times, most recently from a038637 to 3d86bd8 Compare July 8, 2021 16:15
@andy-hook andy-hook marked this pull request as ready for review July 13, 2021 14:12
@benoitgrelard
Copy link
Contributor Author

benoitgrelard commented Jul 13, 2021

@andy-hook this PR is now fully complete right? Just a note that I'd like to review and test this in it's entirety one last time before we merge.

Also, is it up to date with main so we can test the impact of the other fixes like #772?

@jjenzz
Copy link
Contributor

jjenzz commented Jul 13, 2021

@benoitgrelard

image

I assumed it is ready because of that ☝️

Sorry if I jumped the gun with my loop comments though @andy-hook 🙈

@andy-hook
Copy link
Contributor

andy-hook commented Jul 13, 2021

this PR is now fully complete right? Just a note that I'd like to review and test this in it's entirety one last time before we merge.

Yessir. Sounds sensible.

Sorry if I jumped the gun with my loop comments

Not at all, that was my intention when I marked it.

benoitgrelard and others added 11 commits July 29, 2021 11:08
* [FocusScope] Update API for modality changes

* Address PR feedback

* PR feedback
* [Dialog] Add support for non-modal dialogs

* PR feedback
* [AlertDialog] Don't compose onPointerDownOutside

* Code style
* [Popover] Add support for modality

* Modality scroll behaviour, default to non modal

* Adjust for focus close in chromatic stories

* Fix toggle when closing non modal

* [Dialog][Popover] Update interact description
* [Menu] Support modality

* [DropdownMenu] Expose modality

* [ContextMenu] Expose modality

* [DropdownMenu] `mouseDown` -> `pointerDown`

* [Menu] disableOutsidePointerEvents only when open
* Explicit trigger focus

* Simplify handling where possible

* Code style, remove redundant check

* [Popover] Revert `onEscapeKeyDown` default check
…cus behaviour (#799)

* Adjust non-modal auto focus behaviour

* Code style

* Explicitly handle ref clearing

* Refine ref clearing

* Simplify handler composition in non-modal

* Feedback

* Revert auto focus cancellation

* Improve focus handling readability

* [ContextMenu] Adjust non-modal focus handling
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You both know the inner workings on this better than me but just one thing on naming conventions:

)

* Adjust ref naming

* [ContextMenu] Move modal condition
Copy link
Contributor

@jjenzz jjenzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given this a test locally, no obvious issues that I can find so we're gonna get this in Benoit for now.

Definitely raise anything you find when you're back from holibobs though, we can fix separately.

Copy link
Contributor

@andy-hook andy-hook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mirroring what @jjenzz said. It's funny to be in a situation where I'm approving some of my own changes again 😂

@andy-hook andy-hook merged commit 6297e37 into main Aug 2, 2021
@andy-hook andy-hook deleted the modality branch August 2, 2021 16:11
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.

[Dialog] Consider adding disableOutsidePointerEvents [DropdownMenu] Can't enable scroll for DropdownMenu in overflow scroll container
3 participants