Skip to content
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

Merged
merged 24 commits into from
Apr 4, 2023
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Feb 19, 2023

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 let ModalDialog overflows the screen. It is better in terms of bundle size compare to adding new props.

The mental model is like this:

  1. Let's show a modal

    <Modal>
      <ModalDialog>
        …content
      </ModalDialog>
    </Modal>
  2. When content is dynamic and overflows the screen.

     <Modal>
      <ModalDialog>
        <Box sx={{ overflow: 'auto' }}>
          …content
        </Box>
      </ModalDialog>
    </Modal>
  3. The requirement changes, the whole modal dialog should overflow the screen without max-height

  <Modal>
    <ModalOverflow> // just insert this component in between and voila
       <ModalDialog>
         <Box>
           …content
         </Box>
       </ModalDialog>
     </ModalOverflow>
 </Modal>

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 the aria-describedby with the Modal via context.

Same as ModalDialogTitle.


@siriwatknp siriwatknp added component: modal This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy labels Feb 19, 2023
@siriwatknp siriwatknp changed the title [Joy] Add ModalOverflow [Joy] Add ModalOverflow component Feb 19, 2023
@mui-bot
Copy link

mui-bot commented Feb 19, 2023

Netlify deploy preview

@mui/joy: parsed: +0.33% , gzip: +0.25%

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against 6c18b48

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 27, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 27, 2023
@siriwatknp siriwatknp marked this pull request as ready for review February 27, 2023 05:53
Copy link
Contributor

@danilo-leal danilo-leal left a 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).

  1. "Vertical scroll" section within the Modal Dialog
  2. Standalone "Vertical scroll" section
  3. 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.

docs/data/joy/components/modal/ModalDialogOverflow.js Outdated Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 21, 2023

that we can reorganize the content a little

Okay, sounds good.

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.

It's not achievable with sx alone. You need another component, in this case ModalOverflow component, as a wrapper of the ModalDialog.

Developers could do it with Box + sx but ModalOverflow handles both layouts (center and fullscreen) for you.

"expand more on the reasoning" Do you mean to document this internally or put it in the docs?

@danilo-leal
Copy link
Contributor

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 ModalOverflow component conveniently handles both the centered and full-screen scenarios for you, speeding your development a bit" ⎯ or similar!

@siriwatknp
Copy link
Member Author

@danilo-leal I have reordered the demos and adjusted the content of the ModalOverflow section.

I think the ModalOverflow covers most of the use cases so I decided to go with a different suggestion instead. Feel free to adjust as you'd like.

image

Copy link
Contributor

@danilo-leal danilo-leal left a 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?

docs/data/joy/components/modal/modal.md Show resolved Hide resolved
@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 22, 2023

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?

Thanks for asking!

  • Vertical scroll: the scrollable area is within the modal dialog (modal dialog's height will not be more than the viewport).

    Screen.Recording.2566-03-22.at.11.06.03.mov
  • Modal overflow: the modal dialog's height can be bigger than the viewport and when that happens, the whole modal dialog is scrollable.

    Screen.Recording.2566-03-22.at.11.06.23.mov

Developers can decide which experience they want based on their needs.

And similarly, why having the ModalOverflow component is better than the Box approach?

  • It saves time! with Box approach, you will have to copy this styles all the times.
  • It handles click event to close the modal when users click on the backdrop.
  • with ModalOverflow, you can add styleOverrides to the theme in case you have more layout that you want to support.

@danilo-leal
Copy link
Contributor

@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!

@siriwatknp
Copy link
Member Author

@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!

@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 24, 2023

cc @oliviertassinari @mnajdova for more opinions because this component does not exist in Material UI and I'd like to push this forward.

docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
docs/data/joy/components/modal/modal.md Outdated Show resolved Hide resolved
@siriwatknp siriwatknp merged commit 834213c into mui:master Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: modal This is the name of the generic UI component, not the React module! package: joy-ui Specific to @mui/joy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants