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

[Modal] has inline styles instead of classes. Breaks support of classes API #16442

Closed
2 tasks done
burtyish opened this issue Jul 1, 2019 · 14 comments · Fixed by #24857
Closed
2 tasks done

[Modal] has inline styles instead of classes. Breaks support of classes API #16442

burtyish opened this issue Jul 1, 2019 · 14 comments · Fixed by #24857
Labels
component: modal This is the name of the generic UI component, not the React module!
Milestone

Comments

@burtyish
Copy link
Contributor

burtyish commented Jul 1, 2019

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

Overriding a Dialog's style class worked in version 3.x, I expected this to continue working. I didn't see anything about this mentioned in the migration guide, under Dialogs.
I do see the following. Is it related? That's not clear.

[Modal] Remove the classes customization API for the Modal component (-74% bundle size reduction when used standalone).

Current Behavior 😯

The dialog's root div is assigned the class MuiDialog-root, but this class has no CSS styles defined.
The class I assigned via the class API has no effect, since the inline styles have higher specificity.

Steps to Reproduce 🕹

Link: https://codesandbox.io/s/material-ui-dialog-issue-16442-srl0e

  1. Click the button to open two dialogs. The 'important' dialog should have higher z-index.
  2. If you switch the version of material-ui/core to 3.9.3 and repeat you will see the expected behavior

Context 🔦

In my app, I may have more than a single open dialog (yeah, I know it's bad UX, sorry). So this method is what we use to ensure the dialogs are in a certain order.

Your Environment 🌎

Tech Version
Material-UI v4.1.3
React 16.9
Browser Chrome 75.0.3770.100
TypeScript 3.6.0
@oliviertassinari oliviertassinari added the component: modal This is the name of the generic UI component, not the React module! label Jul 1, 2019
@oliviertassinari
Copy link
Member

@burtyish Correct, you have to use !important for overriding the inline style. Your problem is very close to #16441.

I do see the following. Is it related? That's not clear.

Yes, removing the imports to JSS has yielded a significant bundle size reduction. We were at 24 kB gzipped before #15466, we are at 5 kB gzipped now for a single import of the Modal.

@oliviertassinari oliviertassinari self-assigned this Jul 1, 2019
@eps1lon
Copy link
Member

eps1lon commented Jul 2, 2019

We were at 24 kB gzipped before #15466, we are at 5 kB gzipped now for a single import of the Modal.

Just to be clear: You didn't save 19kB in a bundle using material-ui. Only if the Modal was the only component you used.

@burtyish burtyish changed the title Dialog has inline styles instead of classes. Breaks support of overriding Dialog classes. Dialog has inline styles instead of classes. Breaks support of classes API Jul 2, 2019
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 2, 2019

@eps1lon Yes, this statement is accurate. It's slightly bigger actually.

@bengry
Copy link

bengry commented Jul 9, 2019

@oliviertassinari Are there any AI going forward here - i.e. is this something that's going to be changed (ideally), or is the inline-style going to be kept, making it hard to override (force the need to use !important)? It wasn't clear from the thread so far.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 9, 2019

@bengry Which property do you need to override? We might be able to revert by pushing the @material-ui/unstyled package forward #16238.

@bengry
Copy link

bengry commented Jul 10, 2019

@oliviertassinari I wanted to override the z-index, similar to what @burtyish mentioned in the OP.

@jsheetzati
Copy link

jsheetzati commented Jul 14, 2019

I believe I am having a similar issue trying to change the root backgroundColor on the backdrop. Seems like BackdropProps={{ classes: { root: classes.backdropRoot }}} no longer works. Instead have to use className + !important.

https://codesandbox.io/s/material-demo-vm0qu

This previously worked in 3.x and matches the documentation that I see for Backdrop. Is this the new way going forward for modifying the Backdrop style?

@oliviertassinari
Copy link
Member

@jsheetzati Correct.

@oliviertassinari oliviertassinari added this to the v5 milestone Jul 15, 2019
@darkowic
Copy link
Contributor

It is not clearly pointed here and my issue is closed (#16605) - It's not only about overriding styles API but also about using theme's default props override feature. See more in the issue.

@mmaloon
Copy link

mmaloon commented Jul 18, 2019

Popover API docs still indicate a ModalClasses object prop. Can that be removed?

@mmaloon
Copy link

mmaloon commented Jul 18, 2019

Another option to override default z-index is to just pass a style prop to Popover with the required z-index

@oliviertassinari oliviertassinari changed the title Dialog has inline styles instead of classes. Breaks support of classes API [Modal] has inline styles instead of classes. Breaks support of classes API Apr 5, 2020
@oliviertassinari
Copy link
Member

@mnajdova This one might require a tricky tradeoff for the unstyled components. We might want to keep the inline style for simplicity. It seems to be what react-modal does without any major downsides. However, if we do, we make overrides harder, so better be sure the styles rarely need to be overridden.

@mnajdova
Copy link
Member

mnajdova commented Dec 2, 2020

@mnajdova This one might require a tricky tradeoff for the unstyled components. We might want to keep the inline style for simplicity. It seems to be what react-modal does without any major downsides. However, if we do, we make overrides harder, so better be sure the styles rarely need to be overridden.

I agree. I will dig more into it tomorrow and try to prototype how it would look like. Also, just food for thought, if the unstyled component uses style for some portion of the styles it has, is it wrong if developers also use style for overriding it? (This will not be common of course, but if we document it correctly it may help people understand better why that tradeoff was made for some component) If the unstyled component uses className for some styles, then we shouldn't have problem with overriding it anyway..

@mnajdova
Copy link
Member

mnajdova commented Dec 4, 2020

Once we have the separation of unstyled and core versions of the component, I think we will have the best from both worlds ( unstyled version with minimal bundle size) and the styled version with the default styles using emotion/styled-components based on the setup.

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!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants