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

[theme] Always return default spacing value with px units #22552

Merged
merged 7 commits into from
Sep 12, 2020

Conversation

mbrookes
Copy link
Member

@mbrookes mbrookes commented Sep 10, 2020

Breaking changes

  • theme.spacing now returns single values with px units by default.
    This change improves the integration with styled-components & emotion.

    Before:

    theme.spacing(2) => 16
    

    After:

    theme.spacing(2) => '16px'
    

    You can restore the previous behavior with:

    -const theme = createMuiTheme();
    +const theme = createMuiTheme({
    +  spacing: x => x * 8,
    +});

Closes #16205

@mbrookes mbrookes added breaking change core Infrastructure work going on behind the scenes labels Sep 10, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 10, 2020

Details of bundle changes

Generated by 🚫 dangerJS against 44b8dcf

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 10, 2020
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 11, 2020
@mbrookes
Copy link
Member Author

mbrookes commented Sep 11, 2020

width: 300 + parseInt(theme.spacing(6).slice(0, -2));

😄

It's the problem that @joshwooding raised here: #16205 (comment)

@mbrookes
Copy link
Member Author

mbrookes commented Sep 11, 2020

How about if theme.spacing() accepted the unit to append, and treated an empty string as a special case?:

> theme.spacing(1)
'8px'
> theme.spacing([1, 'rem'])
'8rem'
> theme.spacing([1, ''])
8

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 11, 2020

I have tried to benchmark what the others are doing here.

How about if theme.spacing() accepted the unit to append, and treated an empty string as a special case?:

I think that it would leak the intent, so not working.


I wonder if the best answer here isn't on CSS land: calc(). If you take the above example, the motivation is to have 300px and 2 spacing unit (in whatever CSS unit) on each side. @mbrookes Would it work, it seems to?

@mbrookes
Copy link
Member Author

@mbrookes Would it work, it seems to?

Perfect - it even works in IE.

@mbrookes
Copy link
Member Author

Hmm:

padding: theme.spacing(0.5, 0, 0.5, `${Math.max(0, theme.spacing(1) - 3)}px`),

CSS max() isn't supported in IE: https://caniuse.com/?search=max()

@mbrookes
Copy link
Member Author

Also:

    [theme.breakpoints.up(600 + theme.spacing(2) * 2)]: {
      width: 600,
      marginLeft: 'auto',
      marginRight: 'auto',
    },

@oliviertassinari
Copy link
Member

@mbrookes For the first case we can use px, and the second a breakpoint value, md?

@mbrookes
Copy link
Member Author

mbrookes commented Sep 11, 2020

I'm not sure what the second was about (and I wrote it! 😅 ) – it seems to work just the same with 'md'.

I don't understand what you mean for the first - it is in px?

@oliviertassinari
Copy link
Member

I don't understand what you mean for the first - it is in px?

Sorry for the confusion, I meant using hardcoded pixels, not. The value from the theme.

Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
@mbrookes mbrookes merged commit 5b95c39 into mui:next Sep 12, 2020
@mbrookes mbrookes deleted the theme-spacing-px branch September 12, 2020 19:12
@oliviertassinari oliviertassinari added this to the v5 milestone Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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