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

Switch to CSS-in-JS for Styling Purposes #212

Closed
dcwither opened this issue Dec 3, 2020 · 9 comments · Fixed by #220, #226 or #227
Closed

Switch to CSS-in-JS for Styling Purposes #212

dcwither opened this issue Dec 3, 2020 · 9 comments · Fixed by #220, #226 or #227
Assignees

Comments

@dcwither
Copy link
Contributor

dcwither commented Dec 3, 2020

Linaria Styled Components

Linaria allows us to use CSS-in-JS while maintaining zero runtime by adding a compilation step to the application to pull out all the styles into CSS files for runtime usage.

Styled components provides CSS-in-JS. While it has a runtime, it will allow us to use twin.macro in the design system, and in apps making our CSS more consistent.

Motivation

Automatic Bundling

CSS-in-JS makes bundling the design system according the an applications existing bundles easier. It has no need for importing split out CSS in the exact right place since it's already colocated with the JS.

Customization/Theming

Working with Linaria Styled Components would allow us to integrate theming at the compilation phase using an import of styles, as well as at runtime using CSS Variables https://levelup.gitconnected.com/tailwindcss-with-css-variables-513abe2e9a5.

We could accomplish theming with tailwind using CSS variables as well, but Linaria gives us a framework to provide a single point for theming as well as some alternatives if we need to expand beyond CSS variables for theming.

Other options for theming with Linaria are outlined in the Linaria Docs.

Consequences

Inconsistency with Other Team Styling Strategies

Along - Styled Components

Linaria and styled components mostly have similar APIs, so migrating styled-components to Linaria wouldn't be difficult. We would have to make some transitions from themes to CSS variables for greater consistency.

Traject - Global SCSS

We've planned to move away from Traject's global scss for some time (that's part of what the design system is for), so adopting Linaria + Tailwind for future styling would actually be a good option - we may even be able to export the configuration in place of the current styles project.

Grepability of Classes

Using Linaria will remove the grepability of css classes since it uses a hashed classname and even minifies them. This is mitigated by using react dev tools to inspect the components to find the origin of the styles as easily as we did before, and even ensure that we can easily track the full styles applied to these components. We can improve this further by adding explicit component names so that it survives compilation.

Requires additional build step in dependent projects

Because we want the css+js bundling that comes with css-in-js, we would need to provide guidance eon how to build compile out the linaria setup in the app instead of on package publishing.

No Support for CSS Only

Linaria exports minified and hashed classes. While this makes the CSS more performant, it removes the ability to style components using the CSS only options. Currently we are prioritizing the ability to theme, and the bundling benefits over this concern.

Browser Support

If we choose CSS Variables, the global browser support is ~95%. However, looking at our browser matrix, the overall support for our target users is roughly 99.5%. These current numbers are probably due to us already having some browser support issues with other features.

Setup

We'll use Linaria to write the CSS, and incorporate tailwind classes through the use of twin.macro.

CSS Variables

We currently define CSS variables in our design tokens package, we can include variable references to the semantic tokens (i.e. --eds-color-alert-600) and identify ones that will be more dynamic (e.g. --eds-color-background) so they can be modified at runtime.

Twin Macro

First we need to set up the build to use babel instead of typescript. This was already part of the plan.

Then we can add the tw macro to our babel macros. For the most part, the setup can be copied from the emotion setup. The primary exception being we can't use styled from twin.macro because it requires emotion to be in the bundle (this would require a runtime which we don't want).

Global Styles

Tailwind requires a Global Styles to be applied to the application. This shouldn't be applied within the design system due to the risk of it being applied multiple times. Instead the design system will export it for use in the applications where the top level themes are applied.

Styles

If we do adopt Linaria + twin.macro, we'll no longer have a need for our styles package which was going to be our CSS only export. In place of the styles package, we could export the tailwind config for easy use with twin.macro in the applications.

@dcwither
Copy link
Contributor Author

dcwither commented Dec 3, 2020

The main questions to answer is how will theming work and how does that affect the compiled css. Does the app own compiling from css in js to CSS files.

@anniehu4
Copy link
Contributor

anniehu4 commented Dec 8, 2020

Motivation:

  • might be good to call out that we could do theming with Tailwind only, but adding Linaria makes it easier for development / export / [anything else]

Consequences:

this wouldn't be difficult, but would be time consuming

  • Does "this" = migrating to be consistent? Would be good to include why we're choosing Linaria over styled components, given inconsistency
  • Curious about the additional build step -- in traject, do we do that for other dependencies that use styled components? I don't get why it would be different

Setup:

Misc:

  • Could be worth adding a "Cleanup" section that broadly touches on how we could remove the styles/ package, scss tokens, [anything else]

@anniehu4 anniehu4 linked a pull request Dec 8, 2020 that will close this issue
@dcwither
Copy link
Contributor Author

dcwither commented Dec 8, 2020

do we do that for other dependencies that use styled components? I don't get why it would be different

Are there any dependencies that use styled-components? Odds are they're including the runtime - that's one of the reasons we chose linaria - the zero runtime. In order to get that, we would need that compilation step, and it probably shouldn't happen in the design system to make importing and bundling easier.

@anniehu4 anniehu4 linked a pull request Dec 8, 2020 that will close this issue
@dcwither dcwither pinned this issue Dec 8, 2020
@anniehu4 anniehu4 linked a pull request Dec 9, 2020 that will close this issue
@anniehu4 anniehu4 reopened this Dec 10, 2020
@anniehu4 anniehu4 linked a pull request Dec 10, 2020 that will close this issue
@dcwither dcwither assigned dcwither, ahuth and anniehu4 and unassigned dcwither and anniehu4 Dec 10, 2020
@dcwither
Copy link
Contributor Author

Adoption of Linaria and Twin Macro result in failures at the compilation phase. We should prioritize development speed over getting the best performance from the product for now. If Linaria does develop a support story for twin.macro, then we can revisit later.

@ahuth
Copy link
Contributor

ahuth commented Dec 15, 2020

Makes sense. One item I wanted to emphasize:

we can include variable references to the semantic tokens (i.e. --eds-color-alert-600) and identify ones that will be more dynamic (e.g. --eds-color-background) so they can be modified at runtime.

We should explicitly consider the css variables (or a subset of them) as part of the package's public API, and document them somewhere (such as the package's README).

@ahuth
Copy link
Contributor

ahuth commented Dec 16, 2020

Hey @anniehu4 @dcwither couple thoughts on the downsides of styled-components:

  • It's not zero-runtime

    Systems that extract a static stylesheet are nice because once they're downloaded, they're cached and the browser doesn't have to download it again. With styled-components, there's always going to be the runtime cost (likely small) of processing the styles.

  • I've found it harder to debug in Along, at times

    Since there's no stylesheet, you can't use browser devtools to click into a stylesheet and see the raw CSS (such as the order of the styles, etc).

  • Linting doesn't work as well - especially for accessibility

    Linters can't tell what elements are being used. For example

    const MyLink = styled.a` /* some styles */`;
    <MyLink /> // <- jsx-a11y can't tell this is an anchor tag that needs an href
  • To me, applying classnames is much better than creating elements directly

    <button
      className={disabled ? disabledClass : enabledClass}
    />

    vs

    if (disabled) {
       return <DisabledButton />
    }
    return <EnabledButton />

    Obviously personal preference (likely coming from my heavy use of CSS modules at another company).

@anniehu4 anniehu4 reopened this Dec 16, 2020
@anniehu4
Copy link
Contributor

anniehu4 commented Dec 16, 2020

Thanks @ahuth! These are great points, and Linaria definitely addresses them better -- actually 0-runtime, and the syntax supports classname styling: https://github.com/chanzuckerberg/lp-design-system/blob/22a6cbb491e705b5b4412dd13deeb5e9ee701043/packages/components/src/playground/button.tsx#L16-L32

I'm not sure the Linaria implications on debugging / linting, but I assume we can decide on patterns that make those easier.

Devin saw that there's an open PR for adding Linaria support to twin.macro (ben-rogerson/twin.macro#248). I'm a super fan of switching once there's support 🙂 I think we're still early enough that refactoring won't be too bad, and I feel the twin.macro syntax makes it easier

@ahuth
Copy link
Contributor

ahuth commented Dec 16, 2020

Cool, sounds good 👍

I will also point out that styled-components is a popular, reasonable, and still performant choice. And the fact that Along uses it is a strong reason to select it.

@dcwither
Copy link
Contributor Author

Closing in favor of #362

@lesliecodes lesliecodes unpinned this issue Aug 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment