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

Use @emotion library for CSS #71

Closed
wants to merge 9 commits into from
Closed

Use @emotion library for CSS #71

wants to merge 9 commits into from

Conversation

ecgan
Copy link
Member

@ecgan ecgan commented Nov 24, 2020

Changes proposed in this Pull Request:

Closes #65 .

This PR looks into using emotion library to do CSS in JS. Emotion is used in gutenberg.

Some findings from this research:

  • @emotion/css is framework-agnostic, you can use it in HTML with vanilla JS, no react required.
  • @emotion/react is react specific.
  • @emotion/styled is similar to styled-components library. It has a peer dependency on @emotion/react, so you have to manually install @emotion/react even though you are not using it explicitly.
  • @emotion/react and @emotion/css both have a css function but they are not the same! This gave me a gotcha headache. This is briefly mentioned here: https://emotion.sh/docs/package-summary
  • On the official site, it is recommended to use @emotion/react for react project, however I find it generate more noise and more tedious because you either have to modify babel config or put in extra JSX Pragma code at the top of the your JS files. More info here: https://emotion.sh/docs/css-prop . Because of this, I personally do not favor @emotion/react.
  • We can use @emotion/styled to create styled components. An example here: 239d1a3 . I feel this is ok, but my experience tells me we might end up having a lot of StyledXyz component and can be quite a bit of noise and mentally taxing to comprehend and navigate.
  • I'm using @emotion/css library here in this PR. Essentially it generates unique class names that we can use, without having to worry about class name conflict. You can see it's pretty much similar with previous SCSS approach. I think this is a more balanced approach and more welcoming to people who are not familiar.
  • Note that we could have also changed all the manual custom CSS classes (e.g. motivation-text, title, description etc) to use the css function to auto-generate unique class names, so that they wouldn't be affected by CSS from other places (e.g. defined and clashed in global style sheet). I leave them out for now.
  • Because this is essentially JavaScript, an advantage over the SCSS approach is that we can make good use of editor JS support such as "rename variable" and "find all references". i.e. less spelling mistake error.
  • Install vscode-styled-components extension to have better syntax highlighting for CSS in JS in your VS Code editor!

Detailed test instructions:

  1. Checkout the code.
  2. npm install and npm run start.
  3. Go to http://localhost:8888/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fstart and make sure everything still looks ok and same as before.

Changelog Note:

Use emotion library for styling and CSS.

@ecgan ecgan requested review from tomalec, jconroy and a team November 24, 2020 19:00
@ecgan ecgan self-assigned this Nov 24, 2020
Comment on lines +1 to +10
export default {
huge: '1440px',
wide: '1280px',
'x-large': '1080px',
large: '960px', // admin sidebar auto folds
medium: '782px', // adminbar goes big
small: '600px',
mobile: '480px',
'zoomed-in': '280px',
};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this file is the same as https://github.com/WordPress/gutenberg/blob/master/packages/components/src/utils/breakpoint-values.js. This is so that we can use the breakpoints in CSS-in-JS.

@tomalec
Copy link
Member

tomalec commented Nov 24, 2020

Nice and deep research!

Sorry, for being grumpy ;)
Personally, I'm a fan of working canonical/native/vanilla/conventional as far as we can to keep the build, development, and onboarding as simple as possible. Collapsing the stack instead of growing it.

Could you help me build and verify the pros and cons list here?

Pros:

  1. Solves Research on better CSS solution to avoid class name conflict #65 without long class prefixes.
    • guarantees, class names uniqueness
  2. Removes Sass from the stack.
  3. Makes the stack more aligned with Gutenberg.
  4. Lets us use more JS refactoring/IDE tooling, like "rename variable", etc
    BTW, I wonder if that could be addressed with some VSCode extension or CSS variables.

Cons:

  1. Adds something to the stack (build, dev, onboarding).
  2. Requires CSS-in-JS (maybe that's only my old-man's-sentiment).
    (edited after reading the comment below: "we use React / JSX, we are using JS to do HTML + JS. [..] CSS-in-JS [..] can be seen as collapsing the stack.")
  3. Obfuscates class names. Could be somewhat taxing in debugging, once we will process all classes like title, description (If we want to me 100% conflict safe, I'd do that as well)
  4. "[…] emotion that increases the bundle size (and bring no value to us [GB-product-blocks])." Evaluate if we can update to wordpress/components to or 9.0.0 9.2.0 or later woocommerce-blocks#2140 (comment)

I'll take another deeper look tomorrow, as it is pretty late for me.
BTW, phew... I forgot how hard and complicated life is when we are not using Shadow DOM ;)

Edit 2020.11.25 11:12: added performance to the cons, as per comment below

@jconroy
Copy link
Member

jconroy commented Nov 25, 2020

Thanks for exploring this @ecgan , very cool, I haven't had a chance to dive into this space before.

I've done a little digging today and I'm still not quite sure it is something that is being that heavily adopted within A8C just yet. For sure there is some experimentation and components slowly starting to use it more. But it seems like quite a few comments reflect this one

From what I understand for the list of pros and cons above there are also some potential performance downsides (can't cache as well and JS rendering has a little more overhead?) and similar to what Tomek mentioned, the ambiguous class could potentially trip up HEs / making debugging more complicated. I know in the past we've had requests to add more obvious classes and ids to help them on certain projects.

@ecgan
Copy link
Member Author

ecgan commented Nov 25, 2020

@tomalec ,

Sorry, for being grumpy ;)

I'd rather you to be grumpy on me (because it means you've read it and you're talking to me about it), instead of not reading and not grumpy 😉 😄

Personally, I'm a fan of working canonical/native/vanilla/conventional as far as we can to keep the build, development, and onboarding as simple as possible. Collapsing the stack instead of growing it.

A little side story: years ago before I work on react, I was like that too, using HTML, CSS and JS in conventional ways. I was more of an Angular (old and new versions) guy. The first time I looked at React / JSX, I didn't like it because it wasn't "cleanly separated" "pure HTML" and "pure JavaScript". Now, after years of working with React / JSX, I love it and I wouldn't want to go back to conventional (even Angular) ways. To me, the developer productivity gain is massive.

When we use React / JSX, we are using JS to do HTML + JS. With CSS-in-JS, JavaScript is essentially the unifying language that to generate everything HTML + JS + CSS, which can be seen as collapsing the stack.

I just managed to find back a post that I read long time ago, A Unified Styling Language, written in May 2017 with 12k claps now. It provides some very solid reasonings on using CSS in JS. It's a long article, but I strongly recommend reading this one.

There are reasons why such CSS-in-JS libraries are so popular (e.g. styled-components repo with 30k GitHub stars). I do not expect us to adopt this overnight - that would be not realistic and not pragmatic (this is also the reason why I use the @emotion/css's css function with className which is a more balanced approach in this PR). I just wish to implore us to open up and explore the possibilities, play around with it, have a feel of it. If it works, cool. If it doesn't, let's see what else we can do. 🙂

Lets us use more JS refactoring/IDE tooling, like "rename variable", etc
BTW, I wonder if that could be addressed with some VSCode extension or CSS variables.

Do you mean for CSS-in-JS, or pure conventional CSS / SCSS?

In conventional CSS, it would be more difficult because class names in HTML are strings, and I have seen somewhere in the WC code base where there are string concatenations like below (an example):

<div class={`length-${ props.isSuper ? '4' : '8'}`}> // class name can be length-4 or length-8 depending on props.

With string manipulation like this, you can see it would be more difficult to have editor support compared with CSS-in-JS.

BTW, phew... I forgot how hard and complicated life is when we are not using Shadow DOM ;)

Well, this is possibly the next best thing when we can't have Shadow DOM 😆

@ecgan
Copy link
Member Author

ecgan commented Nov 25, 2020

@jconroy ,

From what I understand for the list of pros and cons above there are also some potential performance downsides (can't cache as well and JS rendering has a little more overhead?)

Generally, if I were to make a trade-off between performance and developer productivity (i.e. readability, maintainability etc), I would usually choose the latter. When we optimize for performance, the benefits that we get will become smaller and smaller as technology advances. However, developer / human brain has largely remain the same, so if we choose to optimize for developer, we would give us more benefits in the long run.

I know in the past we've had requests to make add more obvious classes and ids to help them on certain projects.

Got you. With this CSS-in-JS approach, we can still do manual classes or ids to help them out, it can work in harmony 🙂

@jconroy
Copy link
Member

jconroy commented Nov 26, 2020

I've had a few more discussions and done some more digging to help with my thinking.

Given the projects we work on seek to align with Gutenberg's approaches I believe we roughly have 2 paths ahead of us:

1 . Current approach

wc-admin and woo blocks, for the most part, seek to align with current Gutenberg approaches

2. (Potential) Future approach

The G2 project is where Gutenberg is intended to be heading. Elements from this project will potentially begin making their way to Gutenberg early to mid-December this year - subject to community and Gutenberg being accepting of the changes.

Some of the key things you'll notice are:

  • A CSS-in-JS system (powered by Emotion) optimized for the needs of Gutenberg and WordPress - it looks like all wordpress/components have been migrated to use CSS-in-JS within the G2 repo
  • Adjustments to file naming see the Card component Card.js, Card.styles.js

A CSS-in-JS system (powered by Emotion) optimized for the needs of Gutenberg and WordPress

There is a new package @wp-g2/styles which is a wrapper for Emotion that can be used.

I don't know fully what it adds for WP (on top of Emotion that is immediately relevant for us) so it might be overkill for us at the moment but it also might be a good opportunity to help test and provide feedback/contribute.

I actually swapped it in (locally) within this PR and it worked without any further changes on our end. The main thing I noticed is an increase in bundle size of about 70kb.


There is no guarantee everything from G2 will make it to core (or without changes) but it certainly seems like it is trajectory we are headed.


Does any of this help or change your thinking @tomalec @ecgan to progress the discussion ? I don't have a super strong opinion either way but I'd like to move forward on this convo and get things progressing. We can always re-evaluate.

Lets perhaps try and sync up in slack if we can get the 3 of us online at the same time later today.

@tomalec
Copy link
Member

tomalec commented Nov 26, 2020

@ecgan Thanks for sharing your story and a great article!

I'll share a scrap of my story: I started the conventional way, then got into fancy (back then libraries and frameworks), then got back to the Web Platform, and fell in love with Web Components and it's hard for me to imagine why and how one could go back. I hope I'll soon feel in love with React as well.


When we use React / JSX, we are using JS to do HTML + JS. With CSS-in-JS, JavaScript is essentially the unifying language that to generate everything HTML + JS + CSS, which can be seen as collapsing the stack.

Great point, now I totally agree, that given we are already settled in React and with HTML-in-JS, CSS-in-JS is no longer a disadvantage. I'm removing it from my list above.

However, we still need to be careful about being able to provide critical CSS upfront. I'm not sure how to we handle it currently.
Do we SSR? or simply don't care 🙈 ;)


However, I think my "working canonical/native/vanilla/conventional" still stands here, as "-in-JS" is somewhat conventional with React ;)


There are reasons why such CSS-in-JS libraries are so popular

Agree, and the number of reasons is nicely gathered in the article you linked 👍
My only point is that native CSS and Web Platform features are eventually more popular. (no stars to compare, but we can safely assume the superset of stars ;) )

In general, I believe the best code is no code.
Speaking of performance, the developer experience, maintenance, testing, etc.
So, if possible I lean towards limiting dependencies, using lightweight ones, or close to proflyfills.
Like CSS Modules


Do you mean for CSS-in-JS, or pure conventional CSS / SCSS?

I mean pure CSS and CSS in JS. Like CSS Custom properties (--prop:, var() in CSS, element.style.setProperty in JS), plus Houdini, CSS Modules


In conventional CSS, it would be more difficult because class names in HTML are strings, and I have seen somewhere in the WC code base where there are string concatenations like below (an example):

"conventional" is undefined ;) Even with emotion, there is nothing preventing devs from doing so.
So, to prevent such thing's we'd rather need coding guidelines. (And naturally, a nice alternative solution we agree on)

@tomalec
Copy link
Member

tomalec commented Nov 26, 2020

if I were to make a trade-off between performance and developer productivity (i.e. readability, maintainability etc), I would usually choose the latter

In general, I agree. Unless we blow something in the face of a user or HE.
However, when thinking of developer productivity, I mean not only writing code but also inspecting and debugging it in a browser. And personally, I prefer when the HTML and CSS I see in a browser matches the code I have in the repo.

Investigating styles for
.css-1un9gzz.css-1un9gzz.css-1un9gzz.css-1un9gzz.css-1un9gzz.css-1un9gzz.css-1un9gzz (https://g2-components.xyz/?path=/story/components-card--adaptive-height) looks scary to me. I wonder what would be HEs take on that.

@tomalec
Copy link
Member

tomalec commented Nov 26, 2020

Does any of this help or change your thinking @tomalec @ecgan to progress the discussion ? I don't have a super strong opinion either way but I'd like to move forward on this convo and get things progressing. We can always re-evaluate.

Lets perhaps try and sync up in slack if we can get the 3 of us online at the same time later today.

I'm happy to meet and chat :) Just want to note, that, I don't try to act blocky here. I'm just sharing my elaborate, newcomer thoughts and opinions. I'm happy to give Emotion a ride.

@ecgan
Copy link
Member Author

ecgan commented Nov 26, 2020

@tomalec ,

However, we still need to be careful about being able to provide critical CSS upfront. I'm not sure how to we handle it currently. Do we SSR? or simply don't care 🙈 ;)

I don't know either. From what I can see, we are not doing SSR.

I mean pure CSS and CSS in JS. Like CSS Custom properties (--prop:, var() in CSS, element.style.setProperty in JS), plus Houdini, CSS Modules

FYI we are using some of that, an example in http://localhost:8888/wp-content/plugins/google-listings-and-ads/js/build/index.css?ver=1606408694

:root {
  --wp-admin-theme-color: #007cba;
  --wp-admin-theme-color-darker-10: #006ba1;
  --wp-admin-theme-color-darker-20: #005a87; }

which is generated from https://github.com/WordPress/gutenberg/blob/master/packages/base-styles/_default-custom-properties.scss.

@ecgan
Copy link
Member Author

ecgan commented Nov 27, 2020

As per this demo PR #76, because there are many existing things in SCSS that we can't easily port over to JavaScript, we will be putting CSS-in-JS / Emotion library aside and stick to using SCSS way for now.

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

Successfully merging this pull request may close these issues.

Research on better CSS solution to avoid class name conflict
3 participants