[Container] Fix support of custom breakpoint units #23191
Conversation
oliviertassinari
added
bug 🐛
oliviertassinari
changed the title
|
Hey thanks for that, I couldn't spot the Theme thing and I was getting mad! 😃 |
| }; | ||
| } | ||
| return acc; | ||
| }, {}), | ||
| /* Styles applied to the root element if `maxWidth="xs"`. */ | ||
| maxWidthXs: { | ||
| [theme.breakpoints.up('xs')]: { | ||
| maxWidth: Math.max(theme.breakpoints.values.xs, 444), | ||
| maxWidth: Math.max(`${theme.breakpoints.values.xs}${theme.breakpoints.unit}`, 444), |
There was a problem hiding this comment.
Hey! Not sure about this line, I think it is likely to break:
I guess this would work:
maxWidth: `${Math.max(theme.breakpoints.values.xs, theme.breakpoints.unit.endsWith('em') ? 27.75 : 444)}${theme.breakpoints.unit}`,
27.75 comes from dividing 444 / 16 (default font size in browsers).
PS: The syntax is a bit clunky lol
There was a problem hiding this comment.
I am happy to push this changes and open a new PR if there's no other way now that it is merged 😄
There was a problem hiding this comment.
Thanks for double checking. It's definitely broken.
There was a problem hiding this comment.
This behavior is surprising. What is 444? We get the same issue on https://github.com/mui-org/material-ui/blob/3745c1e3a1f65d5a1c13b2d1c9db98bf69bbca33/packages/material-ui/src/Dialog/Dialog.js#L76
There was a problem hiding this comment.
What do you think about simply not handling the case, going back to the previous logic?
There was a problem hiding this comment.
xs is designed to be 0, IMHO, the existing of 444 in the first place is wrong
There was a problem hiding this comment.
Yes xs was designed to be 0 as it is the smallest breakpoint. But in the case of defining containers maxWidth as this case is a 0 won't be a good value I guess that's why the 444 is there. The fix proposed what it does is that it will set the value either to 444px or 27.5em/rem depending on the units being used, or if the breakpoint is different from the default and bigger than 444px/27.5em it will take it as maxWidth which is what the other maxWidth (sm,md,lg...) do...
So when using:
<Container maxWidth={"xs"}>
{children}
</Container>
For the width of the container it will use by default 444px when setting the breakpoints with no units or with pixels. An will use 27.5em for it if they are set in ems...
If we just leave it as it is... It will default use 444px which works for pixels but it won't work well for em/rem units...
There was a problem hiding this comment.
Still It is a codebase which I've recently started working with it and I might be missing something... I think is worth asking someone else in the MUI team to double check what we are discussing :)
There was a problem hiding this comment.
I see now that this bug (at least for sm, md, lg, xl) is showing up across the codebase in some other places... i.e: Dialog Component
There was a problem hiding this comment.
The case of the Dialog is even harder to handle, I doubt we can solve it without custom overrides.
|
Hello! Apart from the changes above. This whole PR is related to #21476 |
|
@espipj The changes will be released before the end of the week in v5-alpha.14. We only backport critical issues. This isn't one. |
When setting breakpoints in different units (like for example
em) the media-query/breakpoints get created properly and they use the units defined.However, the Container component is still using no units at all (defaults to pixels) which makes everything break apart.
This PR should fix it, but I've found out another bug on the ThemeProvider which I may need help with. The units set are not showing up on it at
theme.breakpoints.unitso on the generated CSS it shows up likeundefined:On the React DevTools the Theme provider doesn't show units defined...
If I manually set
unit: "em"under breakpoints through the DevTools, it magically works after that!So basically some tweaks need to be made to the ThemeProvider to make it work, but I can't find where (need some guidance).