Always return value with 'px' unit in the spacing function

[rashdeva]

Hi guys,

We use styled-components as CSS framework and when we call the spacing function with the single argument we have to put 'px' unit in the end, but with multiple arguments, we don't need to do it. It's a little confusing because by changing the number of arguments a developer should add or remove 'px' unit.

Expected Behavior 🤔

Example:

padding: ${props => props.theme.spacing(2)};
padding: ${props => props.theme.spacing(1, 2)};

Result

padding: 16px;
padding: 8px 16px;

Current Behavior 😯

Example:

padding: ${props => props.theme.spacing(2)};
padding: ${props => props.theme.spacing(1, 2)};

Result

padding: 16;   <---- Here is no PX unit
padding: 8px 16px;

So, now when we use single argument we always put 'px' in the end as the example below:

padding: ${props => props.theme.spacing(2)}px;

[oliviertassinari]

@rash2x We have people doing transformations with it: padding: theme.spacing() * 2,. Changing the behavior would be a breaking change. We can consider it for v5.

For now, you can do the following:

const theme = createMuiTheme({
  spacing: x => `${x * 8}px`,
});

[emirotin]

+1 on this, it's really easy to mix up things with the current behavior
Example:

margin: `${theme.spacing(1.5)}px ${theme.spacing(2)}px`

Here I must put px otherwise the generated CSS would be invalid.

Later I decide to refactor it and make a quick change:

margin: `${theme.spacing(1.5, 2)}px`

Now it's invalid again, for a different reason (double pxpx).
Obviously, it'd be my mistake but it's easy to do so.

[oliviertassinari]

@emirotin Please upvote the issue, it will make it more likely to land in v5 :)

[emirotin]

Done :)
Just another example to justify this position. Let's say I have a rule:

margin: theme.spacing(5)

which works with a bare number just fine because of JSS. Later I decide to make it 40px/auto and change it to:

margin: `${theme.spacing(5)} auto`

And now it doesn't work.

[joshwooding]

This change would break theme.spacing(4) + 1 It’s just going to be a trade off

[emirotin]

Alternatively MUI can stop adding 'px' altogether and rely on the jss space-separated syntax: margin: [[5, 10]]

[oliviertassinari]

@emirotin I think that we should optimize for styled-components not JSS.

[emirotin]

My idea was under the hood you're using JSS so relying on its native features sounds logical. Also given you allow passing the custom jss instance to the provider if I'm not mistaken.
For instance, in JSS one can configure the default unit for length to be rem for instance (and that could make sense for responsiveness) while you're currently hard-coding px. Dropping the units from your method and producing this double array would serve more goals IMO:

• it works unitless when used naked thanks to JSS and is compatible with the JSS feature to configure custom units
• is backward-compatible with the old behavior (pre-v4/current single value) and thus allows extra computations
• consistent across usages and consistent with JSS
• now also allows extra computations on multiple units: [theme.spacing(2, 3, 4, 5)[0].map(x => x + 1)]

It will basically be backward-compatible in almost every normal usage:

• single value: behaves as now
• multiple values: when used directly as padding: theme.spacing(2, 4) it keeps working even though the produced value is different
  The only breaking case would be some exotic usage like theme.spacing(2, 4).replace(/px/g, 'em')

[oliviertassinari]

@emirotin The plan is to change the default style engine to styled-components in v5, while keeping JSS for backward compatibility. We need a strategy that works with the majorities of the styling solutions. This array feature comes in a JSS plugin we don't load to minimize bundle size.

[pachuka]

This would still be overridable correct? We like to configure spacing to use rem instead of px and would like to continue to be able to do that.

spacing: (factor) => `${0.5 * factor}rem`,

[oliviertassinari]

@pachuka Yes, we would still allow developers to change the unit.

[oliviertassinari]

This change would break theme.spacing(4) + 1 It’s just going to be a trade off

Would this be good enough?

-padding: theme.spacing(4) + 1
+padding: `calc(${theme.spacing(4)} + 1px)`,