-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[docs] Add docs page for unstyled Modal #31417
Conversation
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
@samuelsycamore @danilo-leal can we do a final review here? |
|
||
## Nested modal | ||
|
||
> ⚠ **Note:** though it is possible to create nested modals—for example, a select modal within a dialog—stacking more than two at a time is discouraged. |
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.
stacking more than two at a time is discouraged.
Why? 😬
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.
Because they block the interaction with the elements behind them. If there are four stacked modals, and you want to go to back to the second one, you need to close two modals and make the interaction you did previously to get to the same state.
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.
> ⚠ **Note:** though it is possible to create nested modals—for example, a select modal within a dialog—stacking more than two at a time is discouraged. | |
> ⚠ **Note:** though it is possible to create nested modals—for example, a select modal within a dialog—stacking more than two at a time is discouraged. The main reason is because they block the interaction of elements behind them. If you stack too many, it will be needed to close all of them in order to get to the root state. |
Nice! Tried to add the reasoning somehow to this note—feel like it's very worth it. Let me know what you and @samuelsycamore think! May need a bit of tweaking to get the words right.
Co-authored-by: Sam Sycamore <[email protected]>
Co-authored-by: danilo leal <[email protected]>
I am putting this on hold, will resume once #31923 is merged. |
This is ready for final review :) |
docs/data/base/pages.ts
Outdated
pathname: '/base/components/utils', | ||
subheader: 'utils', | ||
children: [{ pathname: '/base/react-click-away-listener', title: 'Click-away listener' }], | ||
children: [ | ||
{ pathname: '/base/react-click-away-listener', title: 'Click-away listener' }, | ||
{ pathname: '/base/react-modal', title: 'Modal' }, | ||
], |
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 wonder if it wouldn't make more sense to have the Modal sit within the Feedback sub-category?! I mean, you could think of everything in the Base package as utils, but the modal is ultimately about building feedback sort-of components.
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.
Interesting, in my opinion it is consider a util, because it is used for creating other components. You won't even likely as a developer use it directly in your application, you would use it for creating a dialog, alert or popovers etc. Does that makes sense?
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.
It does but still, you could think of any Base component as the foundation for more components, so in that sense, Base is essentially used for building other components. Therefore, everything could potentially be seen an util, I guess. ClickAwayListener
is a very good example of a pure util but Modal is the foundational component of several others.
I guess that having it under Feedback would also help somehow with discoverability.
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.
Alright, let's cc @michaldudak @siriwatknp for an opinion :)
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.
It does but still, you could think of any Base component as the foundation for more components
The difference between Modal and, say, ButtonUnstyled is that you only need to provide styles for ButtonUnstyled and it's good to go. Modal is more of an internal building block.
I guess that having it under Feedback would also help somehow with discoverability.
I don't think we want to "promote" Modal. We generally encourage devs to use Dialog as that's what they usually require (there's even a note in the docs: "If you are creating a modal dialog, you probably want to use the Dialog component rather than directly using Modal")
Co-authored-by: Sam Sycamore <[email protected]>
Preview - https://deploy-preview-31417--material-ui.netlify.app/base/react-modal/
TODOs:
Resolve API generation(wait for [docs] Migrate button demos to base #31395 to land)Note:
We need to adjust the API a bit,
BackdropComponent
andBackdropProps
should be part ofcomponents
&componentsProps
. Will resolve it in a follow up PR.