Use CSS Modules

[dcwither]

Problem

CSS in JS hasn't been working great for us given our updated constraints:

• Want to encourage tailwind for consisntent css (pattern should be usable in application)
• Want to maintain low specificity, and avoid explosive growth in style variations
• Want meaningful classnames for development, and small classnames for production (minification)
• Easy incorporation of tailwind and plain css

Solution

Mostly back to where we started with CSS, but now that we don't need to consider using global css - we can use css modules, and in order to bind classnames to the styles we'll use classnames/bind.

What does this look like?

Option 1: kebab-case + classnames.bind + string interpolation

/* component.module.css */
.button {...}

.variant-flat {...}
...

// component.tsx

import classNames from "classnames/bind";
import styles from "./clickable.module.css";

const cx = classNames.bind(styles);
// ...
function Clickable({color, size, variant, children, ...rest}) {
  return (
    <button
      className={cx(`button`, `variant-${variant}`, `color-${color}`, {
        [`size-${size}`]: variant !== "link",
      })}
      {...rest}
    >
      {children}
    </button>
  );
}

Option 2: kebab-case + classnames + string interpolation

/* component.module.css */
.button {...}

.variant-flat {...}
...

// component.tsx

import classNames from "classnames";
import styles from "./clickable.module.css";
// ...
function Clickable({color, size, variant, children, ...rest}) {
  return (
    <button
      className={cx(
        styles.button, 
        styles[`variant-${variant}`], 
        styles[`size-${size}`]],
        // ...
      )}
      {...rest}
    >
      {children}
    </button>
  );
}

Option 3: camelCase + classnames + conditionals #365

/* component.module.css */
.button {...}

.variantFlat {...}
...

// component.tsx

import classNames from "classnames";
import styles from "./clickable.module.css";

// ...
function Clickable({color, size, variant, ...res }) 
  return (
    <button
      className={classNames(
        styles.button, 
        size === 'small' && styles.sizeSmall,
        size === 'medium' && styles.sizeMedium,
        size === 'large' && styles.sizeLarge,
        variant === 'flat' && styles.variantFlat,
        variant === 'outline' && styles.variantOutline,
        variant === 'link' && styles.variantLink,
        // ...
      )}
      {...rest}
    />
  );
}

Option 4: camelCase + classnames + capitalized interpolation 🤢

// component.tsx

import classNames from "classnames";
import styles from "./clickable.module.css";

// ...
function Clickable({color, size, variant, ...res }) 
  return (
    <button
      className={classNames(
        styles.button, 
        styles[`size${capitalize(size)}`],
        styles[`variant${capitalize(variant)}`],
        // ...
      )}
      {...rest}
    />
  );
}

Additional Notes:

• babel-plugin-react-css-modules was explored, but in our case this adds another compilation requirement and further separates what's happening from what's being done.
• classnames isn't very useful until there's a lot of conditionals, better to use styles['some-class'] in this scenario

Benefits

• Easiest integration with tailwind through postcss
• Can (if needed) compile to global css through adding prefixes etc.
• No runtime
• Most familiar to the engineers developing it, and using it in the platform
• Works better with the over specified css in the platform
• Meaningful class names in snapshot tests

Challenges and Mitigations

Compilation on App Side

Will need to ensure the css is compiled properly in the app side. This should only involve a suggested addition to a webpack config/storybook config/jest config for the dependent apps.

[ahuth]

Thanks for typing this up. I'm going to leave some thoughts here in separate comments.

Also, we could consider using Github Discussions for things like this in the future (which I believe let you have threads off of comments).

[ahuth]

What does classnames/bind get us over treating the imported styles like normal classes (which they are)?

Could write

// component.tsx

-import classNames from 'classnames/bind';
import styles from './component.module.css';

-const cx = classNames.bind(styles);

function Component() {
-  <div classNames={cx('some-class')} />
+  <div className={styles['some-class']} />
}

[ahuth]

I'm not super familiar with https://github.com/gajus/babel-plugin-react-css-modules. Having briefly glanced through its readme, seems like it adds a styleName attribute? I'd rather use "normal", standard React APIs (such as className).

Less magic = More better.

[ahuth]

Also, I'd encourage the use of camel case class names. For example

.someClass {
  property: value;
}

// component.tsx

import styles from './component.module.css';

function Component() {
  return <div classNames={styles.someClass} />
}

Slightly less typing.

[dcwither]

The main reason for not using camel case was to implement some form of BEM so we write our classes like this block__element--modifier but since the files should remain mostly minimal, I'm open to avoiding that. The other factor is that classnames handles the concatenation for us and allows us to do checks without dealing with ternaries etc.

For the styleName piece, I'm pretty solidly against it at this point. Having used both now, I can confidently say the added syntactic sugar isn't worth the real added complexity.

[ahuth]

Regarding ternaries, I'm completely on board with using classnames.

This totally makes sense

className={cz(styles.foo, active && styles.active, enabled && styles.enabled)}

What's not clear is whether .bind is worth the extra step.

[dcwither]

Summarizing offline discussion outcomes:

• will merge initial changes as is with the bem styling
• will remove the prefixes and will use the following patterns in styles (for button)
  • .button will stay the same
  • .button--variant-outline will become .variantInfo
  • .button--size-large will become .sizeLarge
  • more complex components we can add prefixes (e.g. .modal__header--color-info will become .headerColorInfo

Follow up question: Is this sufficient when considering pages in the app if we want to follow the same style there?

[dcwither]

I updated the issue, but wanted to call out that at least with interpolation camel case makes this much worse (option 4). This leaves options 1,2,3 - I still mostly like option 1, but am happy to go with option 3 if that's the team's preference.

Thoughts?

[anniehu4]

I also think any of options 1-3 could work fine 😛 for option 2 traject has enough 2+ word classNames that I think the overhead of writing styles[] each time is not trivial you know what I tried this out and I eat my words, it doesn't seem like a big deal lol

some other things I thought of:

• Does classnames.bind make it harder to support CSS modules and global CSS in the same file (since we may need to import both/somehow differentiate them)? If so, this may be a nonstarter for now
• 1 trade-off of interpolation is it works well for us for these atomic components (b/c they have to handle a lot of variation -- this is esp. apparent in the typography component), but is less important to larger components / in apps. So option 3 might feel annoying now but be much less annoying soon
• another trade-off is interpolation is easier to write, but I actually think option 3 is slightly easier to read.
• If the type component in refactor(classnames): move to camelcase classnames #365 is the worst it gets in terms of repetitiveness, I think we can live with option 3 -- and there's probably some chunking we can do of sub-pieces to make it better (e.g. maybe we go back to styleFromSize, styleFromColor 😂 )

[ahuth]

Option 3 looks clearer, more explicit, and more obivously correct to me. But that could just be personal preference.

To answer one of your questions, Annie:

Does classnames.bind make it harder to support CSS modules and global CSS in the same file (since we may need to import both/somehow differentiate them)?

I don't believe it makes it harder (correct me if that's wrong). More that it's not clear (to me) that we get a lot of benefit from it.

[dcwither]

Does classnames.bind make it harder to support CSS modules and global CSS in the same file (since we may need to import both/somehow differentiate them)?

I don't think it would be an issue except in the case of collisions between global and local in a given JS/CSS module pair. classnames.bind AFAICT defaults to the identity mapping if there is no hashed mapping.

[dcwither]

The PR is merged, and it seems we've come to an agreement around Option 3. Thanks for reviewing everyone!

[ahuth]

Calling this one accepted 👍