Enable Tailwind utility and component classes #675

jinlee93 · 2021-10-20T22:40:16Z

Summary:

Enables utility and component tailwind classes
As per conversation with Annie:

some of the paths forward are

prefix everything

scope tailwind

live with conflicts and if need higher specificity than the conflict, use scope and use @apply with the class
After digging some in the bootstrap-sass code, we think the chance of conflict is actually pretty low. Relevant files:

https://github.com/twbs/bootstrap-sass/blob/master/assets/stylesheets/bootstrap/_utilities.scss

Component classnames like .text- (https://github.com/twbs/bootstrap-sass/blob/master/assets/stylesheets/bootstrap/_type.scss#L90)

Next steps with this approach:

Bring in utilities directive to traject. We could add it directly to tailwind.base.css OR repurpose tailwind-sr-only.css and rename it tailwind.utilities.css to handle all utility-related things

QA to check for other glaring conflicts

Create guidance about using utility classes. Includes guidance on known existing conflicts (.invisible, .hidden) &

what to do if someone finds a conflict we didn't know about

Add https://www.npmjs.com/package/eslint-plugin-tailwindcss to lint utility classes. (Note: we could use this to ban bootstrap utilities & conflicting classes with the no-custom-classname rule, but that may be too aggressive given all the custom classnames that currently exist)

Decided to go with path 3. of living with the occasional conflict since there shouldn't be many.
My approach to dealing with conflicts is to use the @apply directive or custom styling to scope styling into a different class name.
Examples and details are given in the components readme (./packages/components/README.md)

Test Plan:

CI tests, especially visual regression tests pass

shortcut-integration · 2021-10-20T22:40:19Z

This pull request has been linked to Shortcut Story #141242: Enable Tailwind utility classes.

codecov · 2021-10-20T22:40:33Z

Codecov Report

Merging #675 (50172bb) into main (f91156d) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #675   +/-   ##
=======================================
  Coverage   90.62%   90.62%           
=======================================
  Files          23       23           
  Lines         384      384           
  Branches       96       96           
=======================================
  Hits          348      348           
  Misses         36       36

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f91156d...50172bb. Read the comment docs.

github-actions

size-limit report

Path	Size
components	56.05 KB (0%)
styles	244.2 KB (+98.4% 🔺)

jinlee93 · 2021-10-20T22:48:36Z

size-limit report

Path Size
components 56.05 KB (0%)
styles 244.2 KB (+98.4% 🔺)

🤔 addition of TW utility classes def does increase the size

anniehu4 · 2021-10-21T14:57:55Z

Path Size
components 56.05 KB (0%)
styles 244.2 KB (+98.4% 🔺)

🤔 addition of TW utility classes def does increase the size

oof yeah 😬 also that math is weird to me lol, looks like the current size of styles is 3.91 KB, which is a pretty big increase

2 thoughts:

Is the build noticeably slower? (I looked at a few recent build steps in CI and it seemed only the a11y step was 30s slower, but that could be a fluke on this one)
Maybe we should turn on jit mode for this repo before making this change? (and similarly in traject, turn on jit mode before adding the utilities -- I'm more worried about the size increase there)

packages/components/README.md

anniehu4 · 2021-10-21T15:02:23Z

packages/components/README.md

+    <div class="hidden md:block lg:inline">
+    ```
+    - Where the expectation is `display: none ` for breakpoints smaller than 768px, `display: block `for 768px - 1023px, and `display: inline ` for >= 1024px. However since Bootstrap applies the `!important` property, all screen sizes show `display: none`.
+    - This can be circumvented with the `@apply` directive or using custom CSS with regular media queries to scope styling into a different class name.


let's provide an example to make it easiest to understand

Done, thanks!

github-actions

size-limit report

Path	Size
components	56.05 KB (0%)
styles	4.16 KB (+6.06% 🔺)

jinlee93 · 2021-10-21T17:42:49Z

size-limit report

Path Size
components 56.05 KB (0%)
styles 4.16 KB (+6.06% 🔺)

Ok with the purging, the size drops a lot, is 4.16kb reasonable? @anniehu4

jinlee93 · 2021-10-21T17:43:19Z

With JIT mode on, storybook local build is about 4 seconds faster

packages/components/tailwind.config.js

anniehu4 · 2021-10-21T17:50:36Z

SWEET, yeah that's super reasonable 🙌 I'm fine with adding JIT mode here or in a separate PR

size updated

github-actions

size-limit report

Path	Size
components	56.05 KB (0%)
styles	4.03 KB (+2.94% 🔺)

jinlee93 · 2021-10-21T19:05:50Z

Test pr: #678

anniehu4

so excited!! 1 last small comment & suggestions for the README but overall looks great

packages/components/tailwind.config.js

ahuth · 2021-10-21T19:23:32Z

packages/components/src/styles/global.css

+ * This injects Tailwind's component classes and any component classes
+ * registered by plugins.
+ */
+@tailwind components;


My read of https://tailwindcss.com/docs/extracting-components#extracting-component-classes-with-apply is that this is where the @apply-ed stuff goes.

But I thought we were already getting those in the compiled css? 🤔

Is it possible we're now duplicating anything from @apply?

I think Jin noted that the components.css class contains really few things: https://unpkg.com/[email protected]/dist/components.css. My read of that section was that you can use @layer to control specificity, but it doesn't overlap with the content of @tailwind components

And if the component classes are not being used, they should be purged out

Yeah you're right. Looks like @apply stuff gets inlined right into the .module.css file, not put in components.

ahuth · 2021-10-21T19:26:46Z

packages/components/src/styles/global.css

+ * This injects Tailwind's utility classes and any utility classes registered
+ * by plugins.
+ */
+@tailwind utilities;


Hadn't thought of this until now, but another option is to allow utility classes in stories, but only use @apply in component styles. (And definitely allow utility classes in apps)

Then there's no chance for conflicts and no unfortunate increase in size.

Or put another way:

I think the value in utility classes is in storybook stories and in an app. So we don't necessarily need to actually use them in the design system components if we don't want to.

(If we do want to then that's fine too)

That makes sense to me! It's essentially what we're doing already (since global.css is only used in the storybook file), but we could be more explicit by only purge-ing story files. I'm less worried about the size since according to Tailwind,

When removing unused styles with Tailwind, it’s very hard to end up with more than 10kb of compressed CSS

there is an argument that it's easier to just follow the standard convention though

Separated components and utilities as suggested into tailwindUtilities.css which is imported into .storybook/preview.tsx for storybook use in eds
Let me know if that's not proper naming conventions

ahuth · 2021-10-21T19:28:00Z

packages/components/src/styles/global.css

+ * This injects Tailwind's utility classes and any utility classes registered
+ * by plugins.
+ */
+@tailwind utilities;


We may want to put the @tailwind stuff in another file. Something like src/styles/tailwind.css.

That way an App already using Tailwind (like Traject is/will?) can use it's own Tailwind stuff instead of importing 2 of them (one from here and one from the app), by only importing src/styles/tailwind.css if it's not already doing so.

We can certainly rename this file. It currently only contains tailwind stuff, and it's actually not even imported in traject because there were issues with including all of the base styles (traject defines its own tailwind.base.css instead)

Ah, okay. I assumed there was other stuff going on in it, but it makes sense that there's not.

Naming it tailwind.css sometime might make it more clear when to (and not to) include it in an app.

github-actions

size-limit report

Path	Size
components	56.05 KB (0%)
styles	4.13 KB (+5.26% 🔺)

Co-authored-by: Annie Hu <[email protected]>

jinlee93 · 2021-10-21T22:05:05Z

EDS storybook:

display: none; @apply lg:flex; workaround: https://614b969675171d003ac6f98a-xclfqqspzi.chromatic.com/?path=/story/admin-components-studentcoursemodificationspanel--normal&globals=measureEnabled:false
text- align https://614b969675171d003ac6f98a-xclfqqspzi.chromatic.com/?path=/story/core-components-pagetitle--standard&globals=measureEnabled:false
.invisible .lg:visible https://614b969675171d003ac6f98a-xclfqqspzi.chromatic.com/?path=/story/core-components-megaphone--info&globals=measureEnabled:false
.visible .lg:invisible https://614b969675171d003ac6f98a-xclfqqspzi.chromatic.com/?path=/story/core-components-megaphone--success&globals=measureEnabled:false

anniehu4 · 2021-10-22T15:36:23Z

packages/components/tailwind.config.js

@@ -1,6 +1,8 @@
 const variableTokens = require("@chanzuckerberg/eds-tokens/lib/json/css-variables-nested.json");
 const staticTokens = require("@chanzuckerberg/eds-tokens/lib/json/variables-nested.json");
 module.exports = {
+  mode: "jit",
+  purge: ["./src/**/*.{js,jsx,ts,tsx}"],


If we only want to use the classes in stories for this repo, and we expect other apps to configure their own purge templates (which is reasonable), we could make this

Suggested change

purge: ["./src/**/*.{js,jsx,ts,tsx}"],

purge: ["./src/**/*.stories.{js,jsx,ts,tsx}"],

to enforce that more

and we could add a comment here about the rationale, e.g. "The main value in utility classes for EDS is in storybook stories. We avoid using them in component styles so there's no chance for conflicts with other libraries."

fixed and added comment above line and in readme

anniehu4 · 2021-10-22T15:39:01Z

packages/components/.storybook/preview.tsx

@@ -1,6 +1,7 @@
 import "@chanzuckerberg/eds-tokens/lib/css/variables.css";
 import "../src/styles/fonts.css";
 import "../src/styles/global.css";
+import "../src/styles/tailwindUtilities.css";


I wonder if it'd be nice to mirror the naming convention in traject?

global.css -> tailwind-base.css (I think we should rename this in traject, will leave a comment in the other PR)

tailwindUtilities.css -> tailwind-utilities.css

+1 This makes sense to me

changed as suggested

github-actions

size-limit report

Path	Size
components	56.05 KB (0%)
styles	4.1 KB (+4.65% 🔺)

* feat(tailwind): enable utility classes and add tw- prefix * feat(tailwind): introduces script to add tw- prefix in css files * feat(tailwind): process components css to add tw- prefixes * revert prefixing script and prefixes * revert tw- prefix for tailwind utility classes * feat(tailwind): enable tailwind component classes * docs(tailwind): describe tailwind class usage * docs(tailwind): revise tailwind utility class usage * feat(tailwind): enable tailwind purge * feat(tailwind): enable tailwind jit mode * feat(tailwind): broaden TW purge scope to incl js jsx ts * feat(tailwind): move import tailwind utilities for storybook only * chore(tailwind): move properties to top f tailwind config object * docs(tailwind): update tw custom class with example * docs(tailwind): update tw conflict description Co-authored-by: Annie Hu <[email protected]> * docs(tailwind): udpate tw example * fix(tailwind): specify tailwind purge and add related doc Co-authored-by: Annie Hu <[email protected]>

Jin Lee added 8 commits October 14, 2021 21:12

feat(tailwind): enable utility classes and add tw- prefix

865cea7

feat(tailwind): introduces script to add tw- prefix in css files

db8cc6f

feat(tailwind): process components css to add tw- prefixes

bc0cdb6

revert prefixing script and prefixes

5210e46

revert tw- prefix for tailwind utility classes

ba65a36

Merge branch 'main' into sc-141242

621edf4

feat(tailwind): enable tailwind component classes

5b06b47

docs(tailwind): describe tailwind class usage

86c29ae

jinlee93 requested review from anniehu4 and a team October 20, 2021 22:40

github-actions bot previously requested changes Oct 20, 2021

View reviewed changes

docs(tailwind): revise tailwind utility class usage

bc1f3aa

anniehu4 reviewed Oct 21, 2021

View reviewed changes

packages/components/README.md Outdated Show resolved Hide resolved

anniehu4 reviewed Oct 21, 2021

View reviewed changes

Jin Lee added 2 commits October 21, 2021 10:34

feat(tailwind): enable tailwind purge

b358d26

feat(tailwind): enable tailwind jit mode

dac0e7e

github-actions bot reviewed Oct 21, 2021

View reviewed changes

anniehu4 reviewed Oct 21, 2021

View reviewed changes

packages/components/tailwind.config.js Outdated Show resolved Hide resolved

feat(tailwind): broaden TW purge scope to incl js jsx ts

daded58

github-actions bot reviewed Oct 21, 2021

View reviewed changes

anniehu4 approved these changes Oct 21, 2021

View reviewed changes

packages/components/tailwind.config.js Outdated Show resolved Hide resolved

ahuth reviewed Oct 21, 2021

View reviewed changes

Jin Lee added 3 commits October 21, 2021 13:25

feat(tailwind): move import tailwind utilities for storybook only

8410af4

chore(tailwind): move properties to top f tailwind config object

a9ec8ec

docs(tailwind): update tw custom class with example

b0399db

github-actions bot reviewed Oct 21, 2021

View reviewed changes

jinlee93 and others added 2 commits October 21, 2021 14:26

docs(tailwind): update tw conflict description

948846f

Co-authored-by: Annie Hu <[email protected]>

docs(tailwind): udpate tw example

a251681

anniehu4 approved these changes Oct 22, 2021

View reviewed changes

fix(tailwind): specify tailwind purge and add related doc

84361f6

github-actions bot reviewed Oct 22, 2021

View reviewed changes

ahuth approved these changes Oct 22, 2021

View reviewed changes

Merge branch 'main' into sc-141242

50172bb

jinlee93 force-pushed the sc-141242 branch from b7d09af to 50172bb Compare October 22, 2021 17:15

jinlee93 merged commit e6185ee into main Oct 22, 2021

jinlee93 deleted the sc-141242 branch October 22, 2021 18:44

	purge: ["./src/*/.{js,jsx,ts,tsx}"],
	purge: ["./src/*/.stories.{js,jsx,ts,tsx}"],

Enable Tailwind utility and component classes #675

Enable Tailwind utility and component classes #675

Conversation

jinlee93 commented Oct 20, 2021 • edited Loading

Summary:

Test Plan:

shortcut-integration bot commented Oct 20, 2021

codecov bot commented Oct 20, 2021 • edited Loading

Codecov Report

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Oct 20, 2021

size-limit report

anniehu4 commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Oct 21, 2021

size-limit report

jinlee93 commented Oct 21, 2021

anniehu4 commented Oct 21, 2021 • edited Loading

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Oct 21, 2021

anniehu4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuth Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anniehu4 Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahuth Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Oct 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

size-limit report

jinlee93 commented Oct 20, 2021 •

edited

Loading

codecov bot commented Oct 20, 2021 •

edited

Loading

anniehu4 commented Oct 21, 2021 •

edited

Loading

ahuth Oct 21, 2021 •

edited

Loading

anniehu4 Oct 21, 2021 •

edited

Loading

ahuth Oct 21, 2021 •

edited

Loading

jinlee93 commented Oct 21, 2021 •

edited

Loading