material-ui icon indicating copy to clipboard operation
material-ui copied to clipboard

Container sx maxWidth being overwritten

Open alansouzati opened this issue 3 years ago • 6 comments

Duplicates

  • [X] I have searched the existing issues

Latest version

  • [X] I have tested the latest version

Current behavior 😯

When using maxWidth inside sx prop it gets overwritten by lg breakpoint

see the bug here...

https://codesandbox.io/s/container-sx-maxwidth-bug-d3uy8r

this happens because of this line:

https://github.com/mui/material-ui/blob/master/packages/mui-system/src/Container/createContainer.tsx#L137

Expected behavior 🤔

Container looks up sx maxWidth before defaulting to lg

Steps to reproduce 🕹

Steps:

  1. Open the code sandbox
  2. Notice that I'm setting sm as the maxWidth inside sx (this is actually supported)
  3. When the component renders, you can see the maxWidth is there but it is actually getting overwritten

Context 🔦

Since the sx prop is supported for container, it is important to let users set the maxWidth there similar to how it works with other components like Box.

Your environment 🌎

System:
    OS: macOS 12.4
  Binaries:
    Node: 16.15.1 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 8.11.0 - /usr/local/bin/npm
  Browsers:
    Chrome: 103.0.5060.134
    Edge: Not Found
    Firefox: Not Found
    Safari: 15.5
  npmPackages:
    @emotion/react: ^11.9.3 => 11.9.3 
    @emotion/styled: ^11.9.3 => 11.9.3 
    @mui/base:  5.0.0-alpha.92 
    @mui/codemod:  5.9.3 
    @mui/docs:  5.9.3 
    @mui/envinfo:  2.0.6 
    @mui/icons-material:  5.8.4 
    @mui/joy:  5.0.0-alpha.39 
    @mui/lab:  5.0.0-alpha.93 
    @mui/markdown:  5.0.0 
    @mui/material:  5.9.3 
    @mui/material-next:  6.0.0-alpha.47 
    @mui/private-theming:  5.9.3 
    @mui/styled-engine:  5.8.7 
    @mui/styled-engine-sc:  5.9.3 
    @mui/styles:  5.9.3 
    @mui/system:  5.9.3 
    @mui/types:  7.1.5 
    @mui/utils:  5.9.3 
    @mui/x-data-grid:  5.13.0 
    @mui/x-data-grid-generator:  5.13.0 
    @mui/x-data-grid-premium:  5.13.0 
    @mui/x-data-grid-pro:  5.13.0 
    @mui/x-date-pickers:  5.0.0-alpha.3 
    @mui/x-date-pickers-pro:  5.0.0-alpha.3 
    @mui/x-license-pro:  5.12.3 
    @types/react: ^18.0.14 => 18.0.14 
    react: ^18.2.0 => 18.2.0 
    react-dom: ^18.2.0 => 18.2.0 
    styled-components:  5.3.5 
    typescript: ^4.6.4 => 4.6.4 

alansouzati avatar Aug 03 '22 04:08 alansouzati

@alansouzati What version of @mui/material are you using? It seems fine on my side https://codesandbox.io/s/container-sx-maxwidth-bug-forked-fjedij?file=/src/App.tsx

siriwatknp avatar Aug 04 '22 02:08 siriwatknp

it is not right in your example. inspect the DOM node you will see the following styles:

MuiContainer-root MuiContainer-maxWidthLg css-cryk2y-MuiContainer-root

notice the MuiContainer-maxWidthLg, it should be MuiContainer-maxWidthMd

alansouzati avatar Aug 04 '22 16:08 alansouzati

Notice that I'm setting sm as the maxWidth inside sx (this is actually supported)

I don't think maxWidth as sm ,md etc is supported inside the sx prop. Hence it is always MuiContainer-maxWidthLg as the default one unless overriden via Container's maxWidth prop.

Docs: https://mui.com/system/sizing/

<Box sx={{ width: '75%' }}> // String values are used as raw CSS.

ZeeshanTamboli avatar Aug 06 '22 09:08 ZeeshanTamboli

Why not? I do think it could and it would be consistent with other api in MUI. In fact, see the PR I sent, the system style function actually process it. The maxWidth function supports this.

alansouzati avatar Aug 07 '22 00:08 alansouzati

it would be consistent with other api in MUI

Which other sizing API?

In fact, see the PR I sent, the system style function actually process it.

Yup, I can see it does process. Not sure how it does it.

If we want to keep the support and process the styles, I think we will have to also:

  • Add the TypeScript typings for these breakpoints.
  • Add docs for it.

ZeeshanTamboli avatar Aug 08 '22 04:08 ZeeshanTamboli

What I mean when it comes to the consistent API is that, for example, I can set width as a system prop or as an sx prop and MUI takes it. By checking the implementation of MUI most components move system props to sx props. Container seemed like an exception. As a consumer, I just assumed that piece was consistent but it was not. My PR is a backwards compatible way to make this consistent.

Finally, I do agree to adding the types for it given that the maxWidth implementation does look into the breakpoints to parse it. You can use "md" as a value for maxWidth inside your Box component and this will be processed. It is just not documented.

alansouzati avatar Aug 08 '22 06:08 alansouzati

@alansouzati we can't generate the class name based on the sx's prop maxWidth, as it can contain integer/px values, percentage etc. In addition it does not support the same API as the prop in the Container. As for adding the same API in the sx prop, it's not worth it in my opinion, it would add additional runtime in all MUI components.

In the Container we have a simpler API for the most common use-case, if you want to use the sx prop, you can, and this is how you would go about it: https://codesandbox.io/s/container-sx-maxwidth-bug-forked-1xop7s?file=/src/App.tsx, but I wouldn't recommend this, as there is a simpler API for the same thing.


You can see this as a difference between the color prop in the Button component vs sx's color prop, they are different because the color prop is a prop in the Button API.

mnajdova avatar Sep 30 '22 08:09 mnajdova