-
Notifications
You must be signed in to change notification settings - Fork 257
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
Feedback after migrating from classy-ui to stitches #6
Comments
This is amazing feedback @CodingDive, thank you so much ❤️
Thanks so much again for the encouragement and feedback! 😃 |
Okay, so I played around with the typing and we can indeed fix this. If we have added tokens you can write it as: css.color.RED_500 // With all the type documentation
css.color('red') // Is still possible when you need to override The problem is pseudo selectors, as you can not express those without a function: css.color.RED_500
css.color('RED_500', ':hover') Would have to be like this... hm... have to think on that 😄 |
Okay, discussed a bit and we need to wait with this. The reason is that having duplicate APIs has a high cost in maintenance, documentation and understanding the library. I certainly see the benefit of having a token documented to what CSS it produces, but at the same time... tokens are really there for you to NOT think about the underlying value. The underlying values should freely be able to change without you worrying about it. So yeah, I still agree with you, it was amazing to have that information there... but yeah, here you have a current state of things at least 😄 Please discuss more if you want to! |
Thank you so much for the detailed response.
I believe so too. css.color.RED_500
css.color('RED_500', ':hover') The only reasonable alternative I see is to go full token-based for props/values but not repeat the mistake of classsy-ui and pass the pseudo-selectors into a function. css.color.RED_500,
css.on(':hover').color.RED_500, When used with data-attributes and selectors see #7, we could optionally get some nice type support when passing in an enum of data-attributes. enum LISTBOX_DATA {
DISABLED_OPTION: '[data-reach-listbox-option][aria-disabled="true"]',
}
// assuming we keep the 'on' function for solely pseudo-selectors and 'when' for data-attributes. Those two could also be merged as a single function whose value gets prepended before the property & token as is currently the case for the second argument.
css.when<LISTBOX_DATA>('DISABLED_OPTION').cursor('not-allowed') This would just make it a bit nicer to work with data attributes as the atomic approach of css.color('RED_500', ':hover') Another thing on my stitches API wishlist would be to make the ordering of the arguments agnostic since we are used to writing css.color(':hover', 'RED_500') Is definitely easier to read for me but it's not a big deal either way. I just wanted to add it here since it would've been very difficult to fix this in |
Hey, I just finished migrating some code from classy-ui to stitches and I thought it'd be a good idea to write up some thoughts.
I know that this is a very early version of stitches so my feedback and criticism might not be valid in a few weeks or days from now.
createCss
config at all times.The fact that no babel plugin is required is amazing. With classy-ui, I had to restart CRA at least twice and restart
tsc watch
+ the TypeScript server in my IDE multiple times after adding or renaming a single token. Whenever I change thecreateCss
config, it works immediately after hitting save.Type experience was a lot better in
classy-ui
. Probably because some properties are still missing as already described in Type assistance for non system-ui properties #4.Going beyond what
system-ui
has to offer was really nice. I really enjoyed thatclassy-ui
had transform/translate tokens baked in and provided some default properties e.gAUTO: 'auto'
for spacing which stitches does not, I had to configure it myself. It was also very nice to have some custom css values that we can no longer have unless one usesutils
. I preferred writingdisplay.HIDDEN
(classy-ui) instead ofdisplay('NONE')
even though they express the same thing. Same withstart
,end
instead offlex-start
orflex-end
. It was a nice mini abstraction on top of CSS. Now, I feel like my code is much closer to css than before.Having inconsistent casing is frustrating and makes the code harder to read for me. The convention is to use CONSTANT_CASING (or however it's called, I usually call it upper snake case 😀), yet some non-system-ui properties support only the default, kebab-cased css property values. Certainly not worth to call
css.textAlign('CENTER' as any)
for consistent casing but it'd maybe be better to UPPER_CASE every value by default?css.compose
yielding not a string came as a surprise. Certain libraries (looking at you react-fontawesome) use theclassName
internally. They tried to.split()
it which obviously failed. I saw in the code thatstitches
uses proxies which should work in most cases. The library shouldn't really deal with theclassName
property anyhow as long as I pass in theicon
as a distinct prop. This might not really be a problem ofstitches
but would be good to document nonetheless. Fixed it by calling toString:<FontAwesomeIcon icon={questionMarkIcon} className={css.compose(css.color('BLUE')).toString()} />
Those were the only things I could think of at the moment. I believe the future is bright for stitches. After having used
styled-components
for a year and having tried many different component- and theming libraries, I now believe thatcss-in-ts
is the way to go. Very excited for its future and thank you again for making the first important steps withclassy-ui
andstitches
in that direction. ❤️The text was updated successfully, but these errors were encountered: