Make it possible to pass in margin to Thumbprint React components

[danoc]

As @tomgenoni builds out revamped Thumbprint Utility classes and Jon K. revisits our spacing system, I think this would be a good opportunity to make it possible to add margins to our Thumbprint React components.

This has been a common pain point that folks have worked around by:

a. Adding a wrapper div. b. Avoiding the React components and using their Sass counterparts

Proposal:

<Button u="mb2 s:ms2">
  Click Me
</Button>

The prop u (for "utility"), added to all of our React components, will allow developers to pass in a whitelisted set of utility classes.

We'd start with margin classes, potentially expanding this to support other classes that affect the space around the component.

[xanderwebs]

I've got two questions here:

1. How exactly do we ensure that the things passed to u are valid utility classes and not classNames styled in other places?
2. How wedded are we to the naming scheme? I meant to comment on the RFC but forgot to ultimately..

[tomgenoni]

How exactly do we ensure that the things passed to u are valid utility classes and not classNames styled in other places?

We'd whitelist the margin classes.

How wedded are we to the naming scheme? I meant to comment on the RFC but forgot to ultimately.

Naming scheme for the utility classes? If so, not wedded but want to balance terseness with ability to compose (string them together).

[xanderwebs]

We'd whitelist the margin classes.

As the list of classes grows, and this pattern propagates to other components, will there be adverse effects on the component bundle size? ie if every component is checking a large list of classes, will the bundle size start to balloon?

[tomgenoni]

As the list of classes grows, and this pattern propagates to other components, will there be adverse effects on the component bundle size? ie if every component is checking a large list of classes, will the bundle size start to balloon?

@danoc may have some in mind but I don't think we'd expand it much further than margins since we don't want consumers to change the innards. I assume the whitelist is a regex check, we'll have to see what the implications are there.

[danoc]

As the list of classes grows, and this pattern propagates to other components, will there be adverse effects on the component bundle size? ie if every component is checking a large list of classes, will the bundle size start to balloon?

All of the React components would be pulling from the same object of whitelisted classes. The object would only exist once in a JS bundle and lookups would be quick (since it's an object).

We could use warning to throw a console warning on development if the class isn't part of the whitelist.

[danoc]

The whitelist would only be 2.6kb. Could optimize further, if needed, by creating a regex instead of a whitelist.

[xanderwebs]

2.6kb is not bad.. as long as we're limiting this to the margin classes it should be okay. I wonder if there are other cases to support though. Positioning perhaps?

[danoc]

Yeah, I could see positioning or display being helpful in the future.

[tomgenoni]

With both of those we'd want to be careful, both could introduce non-obvious changes.

The component could be display:block or display:flex and turning that on/off could be tricky. Positioning can also lead to changes in component's default width. Proceed with caution!

[danoc]

Yeah, that's a good point. 👍

[benknight]

I just came across this issue and thought I'd offer some input. I think a nice API would look something like this:

<MyComponent margin={{ a: 1 }} padding={{ t: 2 }}>example</MyComponent>

where a, h, v, t, r, b, l all map respectively to Tachyons spec.

• Uses friendly prop names like margin and padding.
• No whitelisting necessary, but still prevents developers from passing any className.

Kind of overall a nice declarative approach to allowing certain styling on components, making it clear what IS and ISN'T stylable via props.

Here's my implementation as a Gist: https://gist.github.com/benknight/f2e9a7da093c08bddacb54e32c909c51

We could simplify things a bit by removing breakpoints from the above, and just tell authors if they need that much control they should just use a wrapper div or write a new CSS rule.

Also, I could buy the argument that the object literal syntax is too clunky and verbose and awkward to split into multiple lines since it's inside a prop. If we remove the breakpoints option I think that becomes a non-issue. But in any case I think the most basic version of my suggestion is we use declarative props like margin and padding. So if we don't want to use object literals we could just go with something like <MyComponent margin="ma1" padding="pt2" /> where the possible values are indeed just a whitelist of Tachyons classes.

[tomgenoni]

Thanks @benknight, had two thoughts...

1. I don't think we want to allow padding overrides, following the principle of not letting consumers change internals. The components are built to important specifications, buttons for example, and I don't think we should make it easy to change.
2. I'd prefer to keep the class syntax the same across implementations. In my experience the advantages of atomic really shine once you get past having to look everything up. Introducing a second syntax would make that process tougher and the knowledge less portable.

[webbower]

I kind of like @benknight's idea where the classes would be derived internally from the object literal syntax. It could simplify the API because you wouldn't be maintaining a whitelist of class names. Instead, you could do simpler propType validation:

propTypes = {
  margin: PropTypes.shape({
    a: PropTypes.oneOf([1,2,3,4,5,6,7]),
    h: PropTypes.oneOf([1,2,3,4,5,6,7]),
    // etc
  }),
  //...
};

However, I could see a tradeoff being greppability to for a certain spacing configuration. You'd have to get fancy with your regex to find the right setting, especially since the object literal could be single-line or multi-line.

[benknight]

@tomgenoni agreed about not including padding. I think I just wanted to illustrate how we might be able to use explicit prop names to expose a declarative, limited API for what we want to make available for styling.

Taking a step back and playing devil's advocate for a second, what is the tradeoff of just not doing this and forcing people to use wrapper divs? I can see there being cases like using TP components as children of a display: flex div where DOM structure nesting determines layout. But there are still workarounds for that kind of thing. display: contents could also be a viable solution soon enough

And I don't think we lose the advantage of the terse-but-familiar class syntax if the propType is still derived from that syntax, as @webbower noted.

[danoc]

Taking a step back and playing devil's advocate for a second, what is the tradeoff of just not doing this and forcing people to use wrapper divs?

There are always workarounds, but it definitely makes it more difficult for folks to use these components. This is especially true for engineers that are not well-versed in flexbox if working within a flex container.

---

I think it's important for this API to have the same functionality and naming as the utility classes. The originally proposed u prop makes this easy. If we were to use a non-string approach, this API allows us to support breakpoints:

<Button mb={2} ms={[2, 0, 0]}>
  Click Me
</Button>

I have a slight preference for the string approach over the number/array approach in the example above. The string approach is:

1. Easier to document: we'd only have to document one prop instead of 7 (m, mb, ml, ...). It can piggyback off of the Thumbprint Utility class docs.
2. Quicker for developers to write: ={} and ={[,,,]} are slightly harder to write than ="", IMO.
3. Easier to remember: Since developers will be using classes such as s:mb2 in their components within website, the consistency would make it a little easier for folks to use the u prop.

[benknight]

A u prop has no clear meaning or purpose, which are good qualities for prop name to have. How about a prop called marginClassName which works like className, except it only accepts the limited set of utility margin classnames. This bootstraps the clarity of the standard React prop className while also sort-of being clear what is considered valid input, so you'd have JSX that looks like this:

<div className="flex items-end">
  <Button marginClassName="mr2">Cancel</Button>
  <Button>Submit</Button>
</div>

[webbower]

Taking a step back and playing devil's advocate for a second, what is the tradeoff of just not doing this and forcing people to use wrapper divs?

One downside is a potential excess of unnecessary DOM elements. Google's Lighthouse tool flags too many DOM nodes as a penalty.

However, there's benefit to having wrapper elements or components that serve as layout-centric, providing spacing around the UI component and/or altering the layout of internal components (e.g. a ButtonRow component that lays out Button child components in a special way).

CSS class composition is definitely a plus and so maybe creating a new component that is specifically for applying spacing around a component and supports the special margin classes and maybe any other whitelisted utility classes could also be useful.

Re: @benknight's example in his last comment, I'm leery about supporting the margin classname prop on every custom component, mainly for reasons cited in my first paragraph in this comment. Whatever solution we come up with should encourage less DOM elements to be used.

[danoc]

Re: @benknight's example in his last comment, I'm leery about supporting the margin classname prop on every custom component, mainly for reasons cited in my first paragraph in this comment. Whatever solution we come up with should encourage less DOM elements to be used.

Not sure if I understand this part. How would a margin prop on Thumbprint components encourage more DOM?

[benknight]

CSS class composition is definitely a plus and so maybe creating a new component that is specifically for applying spacing around a component and supports the special margin classes and maybe any other whitelisted utility classes could also be useful

This gave me an idea... If we did something like this, we could create some sort of standard interface for limited styling without having to add a new prop to every component.

import Button from '@thumbprint/tp-ui-react-button';
import { withMargin } from '@thumbtack/tp-ui-react-utility-styles';

const SubmitButton = withMargin(Button).top(2).horizontal(3);

const MyComponent = props => (
  <div>
    <SubmitButton />
  </div>
);

Then we use inheritance to do something like class Button extends ComponentWithUtilityStyles where ComponentWithUtilityStyles acts as the gatekeeper for what styling we want to allow. Just margins now, but maybe other utility stuff in the future e.g. we could add things like withPadding.

Ironically even though this is an interesting problem to think of solutions for, I still vote for just not doing anything and accepting the tradeoffs.

[tomgenoni]

To clarify my original thinking...

1. A very common component adjustment is changing margin.

2. Extra wrapping divs cause confusion. This is particularly problematic when wrapping a flexbox child component, which often necessitates recreating the flexbox properties of the original parent. We trade "clean" React for convoluted HTML.

3. The biggest friction point using atomic is learning the syntax. If you add mt3 and s:mt4 it should be as identical as possible across implementations (assuming the performance doesn't suffer significantly). The more we deviate by optimizing for React the more friction we introduce.

HTML:

<div class="mt3 s:mt4" ...

React:

<Component utility="mt3 s:mt4" ... 
or
<Component margin="mt3 s:mt4" ...

No strong feelings on what to call the prop but I'd argue if we can't get parity like this we shouldn't attempt it.

[benknight]

I think prop that just takes a space-separated list of margin utility classnames is fine. Though, I don't like calling it utility because it leaves it unclear what is valid input and developers will end up trying to pass other non-margin utility classnames to it and then not know why they aren't being applied. margin or marginClassName would be my vote.

By the way where does s:mt4 come from? Is that Tachyons?

[tomgenoni]

mt4 is Tachyons, the s: prefix is Tailwind and indicates breakpoints. https://tailwindcss.com/docs/what-is-tailwind/