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

[Divider] Non-integer values passed to theme.spacing in MuiDivider wrapper result in error when spacing is an array #29479

Closed
2 tasks done
anikcreative opened this issue Nov 3, 2021 · 2 comments · Fixed by #29526
Labels
new feature New feature or request package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@anikcreative
Copy link
Contributor

anikcreative commented Nov 3, 2021

Summary: Non-integer values are provided to the theme.spacing() function, when styling the DividerWrapper component. When the default theme spacing has been overridden with an array, these non-integer values are not valid indexes, and result in an error.


  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

I have defined a custom theme with my own spacing rules, overriding the default spacing with a custom array, i.e.:

...
createTheme({
  ...
  spacing: [ 0, 2, 3, 4, 6, 8, 12, ... ],
});

I mounted a <Divider /> component with children, e.g.:

<Divider>
  Rivers
</Divider>

At first, it seemed the child text content rendered correctly, inside a "wrapper" element within the divider:
o8fBXG4e75.

However, you may notice that, unlike the example in the MUI Docs, there is no padding around the child content wrapper. In the browser's developer console, I was greeted with the following error:

MUI: The `theme.spacing` array type cannot be combined with non integer values.You should either use an integer value that can be used as index, or define the `theme.spacing` as a number. 

hzh3AAtUch

There are two factors contributing to this problem.

In the default styling for the DividerWrapper (see here: https://github.com/mui-org/material-ui/blob/master/packages/mui-material/src/Divider/Divider.js#L166), non-integer values are being passed to the theme.spacing() function. The following excerpt is lines 158-163 in Divider.js:

paddingLeft: theme.spacing(1.2),
  paddingRight: theme.spacing(1.2),
  ...(ownerState.orientation === 'vertical' && {
    paddingTop: theme.spacing(1.2),
    paddingBottom: theme.spacing(1.2),
  }),

However, as I mentioned above, I have overridden the default spacing with a custom array. As a result, values passed to theme.spacing() must now be valid indexes of that array, and are therefore limited to integers. When the DividerWrapper styles attempt to call theme.spacing(1.2), the above error occurs.

Expected Behavior 🤔

  • Even if the theme spacing is an array, the default padding for the divider wrapper should show up correctly.
  • The default DividerWrapper styles, and any spacing values passed to style rules therein, should work correctly regardless of whether the theme spacing is an integer coefficient or a custom array. If it's absolutely necessary for the default value to be "something x 1.2", then we ought to do it this way:
paddingLeft: `calc(${theme.spacing(1)} * 1.2)`,
...

Steps to Reproduce 🕹

View this CodeSandbox for a simple reproduction of this issue:

https://codesandbox.io/s/frosty-matsumoto-4fpgj?file=/src/Demo.tsx

Open the "Console" tab to see the console error

Context 🔦

N/A -- See CodeSandbox link above

Your Environment 🌎

I am running @mui/material version 5.0.6

@anikcreative anikcreative added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 3, 2021
@mnajdova
Copy link
Member

mnajdova commented Nov 3, 2021

Agree, if we support this as an input of the createTheme the components coming from @mui/material should not break. Looks like the BreadcrumbCollapsed (internal component) and the Divider are the only components using decimal values when passed. We should update the logic to get the value and then calculate the decimal to avoid this error.

@anikcreative would you like to create a PR for this?

@mnajdova mnajdova added new feature New feature or request ready to take Help wanted. Guidance available. There is a high chance the change will be accepted package: material-ui Specific to @mui/material and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 3, 2021
@anikcreative
Copy link
Contributor Author

anikcreative commented Nov 3, 2021

Thank you for the rapid response and for digging into it a bit, @mnajdova!

I'd be happy to take a look at it; I'll try to have a PR ready soon (hopefully at some point this week).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: material-ui Specific to @mui/material ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
2 participants