-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
Conversation
3fcc043
to
182a290
Compare
I'm not sure how to handle cases like: |
width: 300 + parseInt(theme.spacing(6).slice(0, -2)); 😄 It's the problem that @joshwooding raised here: #16205 (comment) |
How about if > theme.spacing(1)
'8px'
> theme.spacing([1, 'rem'])
'8rem'
> theme.spacing([1, ''])
8 |
I have tried to benchmark what the others are doing here.
I think that it would leak the intent, so not working. I wonder if the best answer here isn't on CSS land: |
Perfect - it even works in IE. |
Hmm: padding: theme.spacing(0.5, 0, 0.5, `${Math.max(0, theme.spacing(1) - 3)}px`), CSS |
Also: [theme.breakpoints.up(600 + theme.spacing(2) * 2)]: {
width: 600,
marginLeft: 'auto',
marginRight: 'auto',
}, |
@mbrookes For the first case we can use px, and the second a breakpoint value, md? |
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? |
Sorry for the confusion, I meant using hardcoded pixels, not. The value from the theme. |
639b4b4
to
13e72db
Compare
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Breaking changes
theme.spacing
now returns single values with px units by default.This change improves the integration with styled-components & emotion.
Before:
After:
You can restore the previous behavior with:
Closes #16205