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

Feedback after migrating from classy-ui to stitches #6

Closed
CodingDive opened this issue Apr 18, 2020 · 4 comments
Closed

Feedback after migrating from classy-ui to stitches #6

CodingDive opened this issue Apr 18, 2020 · 4 comments

Comments

@CodingDive
Copy link
Contributor

CodingDive commented Apr 18, 2020

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.

  1. The description of properties within the type definition was amazing and something I really miss in stitches. When hovering over any property, you can't even see the value they map to which means I either have to memorize all my design tokens and their respective values, or keep a tab on my createCss config at all times.

image

  1. 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 the createCss config, it works immediately after hitting save.

  2. 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.

  3. Going beyond what system-ui has to offer was really nice. I really enjoyed that classy-ui had transform/translate tokens baked in and provided some default properties e.g AUTO: '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 uses utils. I preferred writing display.HIDDEN (classy-ui) instead of display('NONE') even though they express the same thing. Same with start, end instead of flex-start or flex-end. It was a nice mini abstraction on top of CSS. Now, I feel like my code is much closer to css than before.

  4. 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?

image

  1. css.compose yielding not a string came as a surprise. Certain libraries (looking at you react-fontawesome) use the className internally. They tried to .split() it which obviously failed. I saw in the code that stitches uses proxies which should work in most cases. The library shouldn't really deal with the className property anyhow as long as I pass in the icon as a distinct prop. This might not really be a problem of stitches 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 that css-in-ts is the way to go. Very excited for its future and thank you again for making the first important steps with classy-ui and stitches in that direction. ❤️

@christianalfoni
Copy link
Contributor

This is amazing feedback @CodingDive, thank you so much ❤️

  1. Yes, I completely agree. That was a pretty amazing feature. I am running this issue and hoping it will result in allowing it: Support parsing TSDoc string comments  microsoft/TypeScript#38106. Like, there are ways to get around it by doing things like: css.marginTop(tokens => tokens.SPACE_70) , which would give you the type info.... we could also do css.marginTop.SPACE_70which would indeed also give you the type info I believe. The type info here would include typing for marginTop being a function as well... though it is certainly more straight forward. I would look into that 😄

  2. Yeah, exactly, the experience of getting going and configuring is awesome :)

  3. Yeah, lets see where we are at now with the latest update, where types has been improved 😄

  4. Yes, indeed, we are not doing anything currently on the defaults. We could expose a set of tokens though that would give you a smoother experience out of the box. Also supporting more token categories is absolutely possible. If you want to create a tokens configuration without thinking about current constraints that would be awesome... just create a GIST or something. Then that might end up as a default set of tokens in the library 😄

  5. Ah, yes, I believe uppercasing it all is the best, though I think that tokens should probably be exposed as a property instead of a value to the function as explained above. Let me try this first and then you will get a very nice separation there. Just have to check out if we can bring in the typing :)

  6. Yes! This surprised us as well! The reason it works like this is so that we inject the styling needed at the point of consumption... though this in theory seems smart, it might not necessarily be more performant. But yeah, in discussion, and will document this yes

Thanks so much again for the encouragement and feedback! 😃

@christianalfoni
Copy link
Contributor

christianalfoni commented Apr 22, 2020

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 😄

@christianalfoni
Copy link
Contributor

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!

@CodingDive
Copy link
Contributor Author

CodingDive commented Apr 22, 2020

Thank you so much for the detailed response.

having duplicate APIs has a high cost in maintenance, documentation and understanding the library.

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 stitches would make me put most static data attributes into an enum anyhow. The point here is not necessarily the example and the generic enum argument gimmick, but show the potential possibilities of having an extra function for pseudo selectors. It would also certainly be more readable than the current argument order as addressed below.


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 selectors => property & value from left to right.

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 classy-ui, while I believe the function-based approach of stitches to be flexible enough to swap them around easily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants