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

Setting component prop on Dialog will cause bug #33709

Closed
2 tasks done
ClassicDarkChocolate opened this issue Jul 30, 2022 · 10 comments
Closed
2 tasks done

Setting component prop on Dialog will cause bug #33709

ClassicDarkChocolate opened this issue Jul 30, 2022 · 10 comments
Assignees
Labels
bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module! regression A bug, but worse

Comments

@ClassicDarkChocolate
Copy link

ClassicDarkChocolate commented Jul 30, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Current behavior 😯

The default styles are not set on the DialogRoot if the component prop is specified in @mui/material@5.8.6.

Expected behavior 🤔

The default styles are set on the DialogRoot if the component prop is specified just like in @mui/material@5.8.5.

Steps to reproduce 🕹

https://codesandbox.io/s/sparkling-snow-l15vph?file=%2Fsrc%2FApp.tsx

Context 🔦

No response

Your environment 🌎

npx @mui/envinfo
System:
    OS: Linux 5.10 Ubuntu 22.04 LTS 22.04 LTS (Jammy Jellyfish)
  Binaries:
    Node: 16.15.1 - /tmp/xfs-d0a166eb/node
    Yarn: 3.1.1 - /tmp/xfs-d0a166eb/yarn
    npm: 8.11.0 - /usr/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: Not Found
  npmPackages:
    @emotion/react: ^11.9.3 => 11.9.3 
    @emotion/styled: ^11.9.3 => 11.9.3 
    @mui/base:  5.0.0-alpha.91 
    @mui/icons-material: ^5.8.4 => 5.8.4 
    @mui/lab: ^5.0.0-alpha.92 => 5.0.0-alpha.92 
    @mui/material: ^5.8.6 => 5.8.6
    @mui/private-theming:  5.9.1 
    @mui/styled-engine:  5.8.7 
    @mui/styles: ^5.9.2 => 5.9.2 
    @mui/system:  5.9.2 
    @mui/types:  7.1.5 
    @mui/utils:  5.9.1 
    @mui/x-date-pickers:  5.0.0-beta.3 
    @types/react: ^17.0.48 => 17.0.48 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
    typescript: 4.5.5 => 4.5.5
@ClassicDarkChocolate ClassicDarkChocolate added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 30, 2022
@kulraj710
Copy link

I am looking more into this issue, but the Dialog component relies on Model component which already renders the component <div></div>. (Rendering another component than div would not be required in most of the use cases)

Although docs clearly state that all Props of the Model component should be available to the Dialog component. That's why it shouldn't have broken the design.

@siriwatknp siriwatknp added support: question Community support but can be turned into an improvement component: dialog This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 4, 2022
@siriwatknp
Copy link
Member

Style is set on DialogRoot if component is specified just like in @mui/material@5.8.0.

@ClassicDarkChocolate Are you saying that it used to work in v5.8.0?

@siriwatknp siriwatknp added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed support: question Community support but can be turned into an improvement labels Aug 4, 2022
@ClassicDarkChocolate
Copy link
Author

Style is set on DialogRoot if component is specified just like in @mui/material@5.8.0.

@ClassicDarkChocolate Are you saying that it used to work in v5.8.0?

Yes, it works in 5.x~5.8.4 and breaks after 5.8.5

@adamransom
Copy link

We also noticed this issue in 5.8.5. We are using the ScopedCssBaseline as the component for Dialog, so we get the base-styles applied in an environment where we can't have the global CSS baseline/reset. This used to work perfectly (the styles from both the root modal and ScopedCssBaseline get merged), but now the root modal styles are not applied and only the ScopedCssBaseline styles get applied.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Aug 11, 2022

@adamransom Do you pass the component prop in Dialog?

Is this issue occurring only if the component prop is passed to Dialog? component prop was itself introduced in Modal in v5.8.5. How were you guys using the component prop before v5.8.5?

@ClassicDarkChocolate
Copy link
Author

component prop was itself introduced in Modal in v5.8.5. How were you guys using the component prop before v5.8.5?

https://v5-0-6.mui.com/api/modal/

Name Type Default Description
children* element   A single child content element.⚠️ Needs to be able to hold a ref.
open* bool false If true, the component is shown.
BackdropComponent elementType styled(Backdrop, { name: 'MuiModal', slot: 'Backdrop', overridesResolver: (props, styles) => { return styles.backdrop; }, })({ zIndex: -1, }) A backdrop component. This prop enables custom backdrop rendering.
BackdropProps object   Props applied to the Backdrop element.
classes object   Override or extend the styles applied to the component. See CSS API below for more details.
closeAfterTransition bool false When set to true the Modal waits until a nested Transition is completed before closing.
component elementType   The component used for the root node. Either a string to use a HTML element or a component.

@ZeeshanTamboli
Copy link
Member

ZeeshanTamboli commented Aug 11, 2022

Thanks @ClassicDarkChocolate ! Looks like a bug to me. Styles should get applied to DialogRoot even if component prop is specified. Something to do with changes in #32901 .

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 11, 2022
@adamransom
Copy link

adamransom commented Aug 11, 2022 via email

@ZeeshanTamboli
Copy link
Member

I've created a PR to fix it - #33934

@ZeeshanTamboli
Copy link
Member

Closing this as it got fixed in @mui/material version 5.10.11 with the renaming of components prop to slots prop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: dialog This is the name of the generic UI component, not the React module! regression A bug, but worse
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants