-
Notifications
You must be signed in to change notification settings - Fork 133
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
Implement shorthand syntax for spacing and backgroundColor #22
Implement shorthand syntax for spacing and backgroundColor #22
Conversation
From my own experience, I agree that having these shorthands can be very comfortable once you get used to them. It was a conscious decision however to not include these in Restyle, since it goes against the variable / parameter naming best practices we have at Shopify. We generally want to use clear naming conventions and avoid non-standard acronyms. I'll bring this up as a discussion point with the team, but would you consider rewriting this as something that can be opted in to? I think this could be broken up into a separate |
@JoelBesada What if you just separate the types instead? That way if you don't want shorthand props, it can be excluded on the type level so you get errors when they get passed in. Otherwise the dev experience takes a hit for something like createText if we add it as a param since the user is then forced to pass in the I would think that most users would want to enable the shorthand and that it should be the default. For anyone that wants to disable it, they can opt-out via the types. For example: export type TextProps<Theme extends BaseTheme, EnableShorthand extends boolean = true> =
ColorProps<Theme> &
OpacityProps<Theme> &
VisibleProps<Theme> &
TypographyProps<Theme> &
SpacingProps<Theme> &
TextShadowProps<Theme> &
VariantProps<Theme, 'textVariants'> &
(EnableShorthand extends true ? SpacingShorthandProps<Theme> : never); |
Updated the PR, lmk what you think |
7c466bf
to
2eb2b6e
Compare
src/createBox.ts
Outdated
PositionProps<Theme> & | ||
(EnableShorthand extends true | ||
? SpacingShorthandProps<Theme> & BackgroundColorShorthandProps<Theme> | ||
: never); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can agree on letting the shorthands be enabled by default, and letting anyone opt out via the types. The dev experience for opting out becomes a bit cumbersome with this approach though, since you'd need to pass in |
Its much nicer/cleaner to write out the spacing props in shorthand. This syntax is used in many similar projects that follow the system-ui spec.
True, although that wouldn't be as big of an issue when #19 is addressed. |
2eb2b6e
to
d67a5e7
Compare
LGTM |
This is amazing, I'm a chakra-ui / styled-system / theme-ui addict I was hoping to have those shorthands props ! |
Its much nicer/cleaner to write out the spacing props in shorthand. This syntax is used in many similar projects that follow the system-ui spec.