[Breadcrumbs][Divider] Replace decimal spacing values with integers and css calc

[anikcreative]

Replace decimal values passed to theme.spacing() with integers

• use combination of theme.spacing() and css calc() arithmetic to obtain final value
• components affected: BreadcrumbCollapsed, DividerWrapper
• close [Divider] Non-integer values passed to theme.spacing in MuiDivider wrapper result in error when spacing is an array #29479

• I have followed (at least) the PR section of the contributing guide.

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 442a908

[anikcreative]

N.B. This change affects two components, but I wasn't sure how to reference them both in the PR title (as per contributing guidelines). However, I kept the individual commits separated by component, and mentioned the component names in the commit messages.

[mnajdova]

This looks good to me 👍

[siriwatknp]

Would it make more sense to fix the logic in createSpacing.ts? It seems like the bug is in theme.spacing, not how components use it.

[anikcreative]

I could be wrong here, but it seems to me that the origin of the error message...

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. 

...is this issue: #23187

This seems very intentional to me; i.e. MUI specifically supports an array type for the spacing options, and — when spacing is indeed an array — values passed to theme.spacing should be valid array indexes. If we are saying that non-integer values passed to theme.spacing are always valid, should we revisit issue #23187 and reconsider the error message?

[siriwatknp]

I could be wrong here, but it seems to me that the origin of the error message...

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. 

...is this issue: #23187

This seems very intentional to me; i.e. MUI specifically supports an array type for the spacing options, and — when spacing is indeed an array — values passed to theme.spacing should be valid array indexes. If we are saying that non-integer values passed to theme.spacing are always valid, should we revisit issue #23187 and reconsider the error message?

I got your point.

2 solutions that I see

1. all of the components should never use theme.spacing(decimal) because we force the warning
2. remove the warning and fix createSpacing to support decimal if spacing array is specified.

option 1 will hurt us in the long term because we might miss it in other components and some people will open the same issues, so I favor option 2.

Right now, I am good with this PR because it fixes what we have missed. However, can you create another issue to discuss about the option 2, if that makes sense?

[siriwatknp]

👍 Thanks for your first contribution!

[anikcreative]

option 1 will hurt us in the long term because we might miss it in other components and some people will open the same issues, so I favor option 2.

Right now, I am good with this PR because it fixes what we have missed. However, can you create another issue to discuss about the option 2, if that makes sense?

Totally agree. The theme.spacing() API should not sometimes work as advertised and sometimes not work, depending on certain theme configurations / overrides.

I'll create a new issue as requested, shortly.

👍 Thanks for your first contribution!

My pleasure. Happy to contribute to this wonderful project!

[anikcreative]

Sorry, didn't mean to close the PR with unmerged changes

[oliviertassinari]

For context, we had a couple of discussions in the past on whether it makes sense to use theme.spacing() in the components or not. We default to not use it unless there is an obvious reason for why if the value in the theme is different, then the CSS value used by the component should be different too. For example, <Stack spacing={1}> is a great use case for internal usage of theme.spacing(). But usage in the Button? No, this would have likely more negative side-effects than positives.

[oliviertassinari]

remove the warning and fix createSpacing to support decimal if spacing array is specified.

@siriwatknp

👍 for supporting half values: e.g. 0.5, 1.5. It's very handy when you have to split the spacing in two between margin-top and margin-bottom. It's also easy to compute when spacing is an array, the logic can be: theme.spacing[value * 2] / 2.
👎 for supporting 1.2. I personally think that theme.spacing() could warn against (in all the cases). To me, it doesn't make sense. It's probably better for the developers to hard code the value or uses calc in this case.

Do you want to open a new issue for the system? This could be a great follow-up to improve the DX.