-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Joy] Add ModalOverflow
component
#36262
Conversation
Netlify deploy preview@mui/joy: parsed: +0.33% , gzip: +0.25% Bundle size report |
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.
This looks like a great implementation! I do think, though, that we can reorganize the content a little because there are potentially confusing sections talking about similar outcomes (large content that's vertically scrollable on a modal).
- "Vertical scroll" section within the Modal Dialog
- Standalone "Vertical scroll" section
- And this newly added "Modal overflow" one
Additionally to that, it may be relevant to expand more on the reasoning to extract this out onto an individual component as it seems something that's achievable through the sx
prop.
Okay, sounds good.
It's not achievable with Developers could do it with "expand more on the reasoning" Do you mean to document this internally or put it in the docs? |
I mean mostly inserting into the docs, maybe explicitly like you said: e.g. "You can achieve that using the Box component but using the |
@danilo-leal I have reordered the demos and adjusted the content of the ModalOverflow section. I think the |
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.
Awesome, it's getting better! I'm still a bit confused about the difference between the "Vertical scroll" section, within Layout, and the "Modal overflow" section. What's the difference between them and why would I prefer one over the other? And similarly, why having the ModalOverflow component is better than the Box approach?
Thanks for asking!
Developers can decide which experience they want based on their needs.
|
@siriwatknp awesome, thanks for the explanation! I was sincerely asking from a user perspective as I was trying to understand it myself 😬 I tweaked a bit your latest changes aiming at being as clear as possible. Curious about what you & @samuelsycamore think about it! |
👍 looks better than my version! |
cc @oliviertassinari @mnajdova for more opinions because this component does not exist in Material UI and I'd like to push this forward. |
This component is required to have feature parity with Material UI Dialog.
https://deploy-preview-36262--material-ui.netlify.app/joy-ui/react-modal/#modal-overflow
Why
Joy UI modal components are more low-level than Material UI Dialog. They are one-to-one mapping to the DOM which makes customization with
sx
prop easy.However, to make the whole modal dialog scrollable, another wrapper has to be inserted (this will have the same structure as Material UI Dialog)
I think it is best to use a new component,
ModalOverflow
, as a parent to letModalDialog
overflows the screen. It is better in terms of bundle size compare to adding new props.The mental model is like this:
Let's show a modal
When content is dynamic and overflows the screen.
The requirement changes, the whole modal dialog should overflow the screen without max-height
Further improvement
In step 2, developer could forget to add
overflow: 'auto'
to the content's container because the content is dynamic.We can add a new component,
ModalDialogContent
that has overflow by default and handle thearia-describedby
with theModal
via context.Same as
ModalDialogTitle
.