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

Size shorthand is overwriting an existing CSS property #2166

Open
thecrowkeep opened this issue Mar 15, 2022 · 12 comments · May be fixed by #2341
Open

Size shorthand is overwriting an existing CSS property #2166

thecrowkeep opened this issue Mar 15, 2022 · 12 comments · May be fixed by #2341
Labels
bug Something isn't working

Comments

@thecrowkeep
Copy link

I was mighty confused why my printing stylesheet wasn't working, until I finally discovered that the size property was being replaced with width and height.

size is a property used for defining @page size

I might recommend wh as a shorthand instead, as it describes the format of the string to be passed, is shorter, and is generally more descriptive than size. On top of not conflicting with an existing property.

@hasparus
Copy link
Member

Thanks for the issue @thecrowkeep!

@lachlanjc do you see any solution for it other than getting rid of size shorthand with a breaking change?

@lachlanjc
Copy link
Member

@lachlanjc do you see any solution for it other than getting rid of size shorthand with a breaking change?

Nope, I think that's the only good way to do this

@hasparus hasparus added the bug Something isn't working label Mar 17, 2022
@thecrowkeep
Copy link
Author

Alternatively you could check if it's under the @page media query, as far as I am aware that's the only place the size property is used, and @page ignores width/height properties. So I don't see that being a breaking change as no one would be using the size shorthand under @page.

I'm just not sure about the complexity of implementing something like that, just wanted to throw that out there

@hasparus
Copy link
Member

Alternatively you could check if it's under the @page media query, as far as I am aware that's the only place the size property is used, and @page ignores width/height properties.

It makes sense. Good idea @thecrowkeep!

@benface
Copy link

benface commented Mar 20, 2022

Would it work to rewrite size: x as size: x; width: x; height: x? size: x would simply be ignored outside of a @page context, and width: x; height: x would be ignored in a @page context.

Also, relevant discussion in the CSSWG: w3c/csswg-drafts#820

@hasparus
Copy link
Member

hasparus commented Mar 21, 2022

A workaround to use until we figure out how to solve this:

Opt out of shorthands with css prop. Style objects you pass to css go directly to Emotion, so we're not transforming the shorthand.

Edit: Better workaround in thecrowkeep's post below ⬇

@thecrowkeep
Copy link
Author

Opt out of shorthands with css prop.

This is certainly helpful, though @page is meant to be used in the root styles, so currently the only workaround I'm aware of is:

<Global
	styles={() => ({ '@page': { size: '4in 6in landscape' } }) }
/>

@lachlanjc
Copy link
Member

Oh amazing! Yeah, that’s a much more elegant solution, though let’s document it in the Global styles doc &/or with our shorthands when it’s implemented.

@herrethan
Copy link

herrethan commented Mar 23, 2022

Could this be related to this error I now have as of v0.14.1 around a css definitions conflict?
Happens with all components for me, just using <Link /> as an example:

import { Link, LinkProps } from 'theme-ui';
export const CustomLink = React.forwardRef<HTMLAnchorElement, LinkProps>((props, ref) => {
  return <Link ref={ref} {...props} />;
});
Types of property 'css' are incompatible.
Type 'Interpolation<any>' is not assignable to type 'InterpolationWithTheme<any>'

Or should this be a new bug?

@lachlanjc
Copy link
Member

@herrethan That's definitely not this bug!

@hasparus
Copy link
Member

@herrethan could you create an issue with some reproduction? This bug looks like something that could happen, but I can't recreate it on my side.

@herrethan
Copy link

@hasparus thanks, for some reason only some components now throw this error, so I'm starting to think its related to my environment.

But just to add something I have noticed which sounds related: certain css properties don't play nice in sx without being explicitly cast as ThemeUIStyleObject.

For example

const sx = { color: 'red', display: 'block', left: 5 }
const MyButton = <Button sx={sx} /> // all good

But adding position makes TS barf

const sx = { color: 'red', display: 'block', left: 5, position: 'absolute' }
const MyButton = <Button sx={sx} /> // Type 'string' is not assignable to type 'StylePropertyValue<Position | undefined>'

As long as you cast your sx styles as ThemeUIStyleObject all is well, but it's surprising that you don't always need to. Is that intended because of the way only certain css fields are overridden? If not, I can document this in a new bug ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants