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

[Proposal][style-guide]Tips and best practices for Emotion styled/css implementation #15264

Closed
rusackas opened this issue Jun 18, 2021 · 4 comments
Labels
guidelines Proposals for new or updated guidelines preset-io

Comments

@rusackas
Copy link
Member

rusackas commented Jun 18, 2021

The following is a proposal to provide guidance on how best to use Emotion, some of the tips/tricks/patterns that exist in the repo, and the pros/cons of using styled vs css approaches, with example use cases. This proposal, after a little clean-up here and feedback from this thread, will be posted to the dev@ email list for lazy consensus before being migrated to the Superset Wiki.


Emotion Styling Guidelines

DO use styled when you want to include additional (nested) class selectors in your styles
DO use styled components when you intend to export a styled component for re-use elsewhere
DO use css when you want to amend/merge sets of styles compositionally
DO use css when you're making a small, or single-use set of styles for a component
DO move your style definitions for the css prop to an external variable when they get long
DO prefer tagged template literals (css={css...) over style objects wherever possible for maximum style portability/consistency (note: typescript support may be diminished, but IDE plugins like this make life easy)

DO NOT use styled for small, single-use style tweaks that would be easier to read/review if they were inline
DO NOT export incomplete AntD components (make sure all their compound components are exported)

Emotion Tips and Strategies

The first thing to consider when adding styles to an element is how much you think a style might be reusable in other areas of Superset. Always err on the side of reusability here. Nobody wants to chase styling inconsistencies, or try to debug little endless overrides scattered around the codebase. The more we can consolidate, the less will have to be figured out by those who follow. Reduce, reuse, recycle.

When to use css or Styled

In short, either works for just about any use case! And you’ll see them used somewhat interchangeably in the existing codebase. But we need a way to weigh it when we encounter the choice, so here’s one way to think about it:

A good use of styled syntax if you want to re-use a styled component. In other words, if you wanted to export flavors of a component for use, like so:

const StatusThing = styled.div`
  padding: 10px;
  border-radius: 10px;
`;

export const InfoThing = styled(StatusThing)`
  background: blue;
  &::before {
    content: "ℹ️";
  }
`;

export const WarningThing = styled(StatusThing)`
  background: orange;
  &::before {
    content: "⚠️";
  }
`;

export const TerribleThing = styled(StatusThing)`
  background: red;
  &::before {
    content: "🔥";
  }
`;

You can also use styled when you’re building a bigger component, and just want to have some custom bits for internal use in your JSX. For example:

const SeparatorOnlyUsedInThisComponent = styled.hr`
  height: 12px;
  border: 0;
  box-shadow: inset 0 12px 12px -12px rgba(0, 0, 0, 0.5);
`;

function SuperComplicatedComponent(props) {
  return (
    <>
      Daily standup for {user.name}!
      <SeparatorOnlyUsedInThisComponent />
      <h2>Yesterday:</h2>
      // spit out a list of accomplisments
      <SeparatorOnlyUsedInThisComponent />
      <h2>Today:</h2>
      // spit out a list of plans
      <SeparatorOnlyUsedInThisComponent />
      <h2>Tomorrow:</h2>
      // spit out a list of goals
    <>
  );
}

The css prop, in reality, shares all the same styling capabilities as styled but it does have some particular use cases that jump out as sensible. For example if you just want to style one element in your component, you could add the styles inline like so:

function SomeFanciness(props) {
  return (
    <>
      Here's an awesome report card for {user.name}!
      <div 
        css={css`
          box-shadow: 5px 5px 10px #ccc;
          border-radius: 10px;
        `
      }>
        <h2>Yesterday:</h2>
        // ...some stuff
        <h2>Today:</h2>
        // ...some stuff
        <h2>Tomorrow:</h2>
        // ...some stuff
      </div>
    <>
  );
}

You can also define the styles as a variable, external to your JSX. This is handy if the styles get long and you just want it out of the way. This is also handy if you want to apply the same styles to disparate element types, kind of like you might use a CSS class on varied elements. Here’s a trumped up example:

function FakeGlobalNav(props) {
  const menuItemStyles = css`
    display: block;
    border-bottom: 1px solid cadetblue;
    font-family: "Comic Sans", cursive;
  `;

  return (
    <Nav>
      <a css={menuItemStyles} href="#">One link</a>
      <Link css={menuItemStyles} to={url} >Another link</Link>
      <div css={menuItemStyles} onclick={alert('this is not a great example`)} >Another link</Link>
    </Nav>
  );
}

css tips and tricks

Using the theme

css lets you write actual CSS

By default the cssprop uses the object syntax with JS style definitions, like so:

<div css={{
  borderRadius: 10;
  marginTop: 10;
  backgroundColor: `#00FF00`
}}>Howdy</div>

But you can use the css interpolator as well to get away from icky JS styling syntax. Doesn’t this look cleaner?

<div css={css`
  border-radius: 10px;
  margin-top: 10px;
  background-color: `#00FF00`;
`}>Howdy</div>

You might say “whatever… I can read and write JS syntax just fine.” Well, that’s great. But… let’s say you’re migrating in some of our legacy LESS styles… now it’s copy/paste! Or if you want to migrate to or from styled syntax… also copy/paste!

You can combine css definitions with array syntax

You can use multiple groupings of styles with the css interpolator, and combine/override them in array syntax, like so:

function AnotherSillyExampe(props) {
  const shadowedCard = css`
    box-shadow: 2px 2px 4px #999;
    padding: 4px;
  `;
  const infoCard = css`
    background-color: #33f;
    border-radius: 4px;
  `;
  const overrideInfoCard = css`
    background-color: #f33;
  `;
  return (
    <div className="App">
      Combining two classes:
      <div css={[shadowedCard, infoCard]}>Hello</div>
      Combining again, but now with overrides:
      <div css={[shadowedCard, infoCard, overrideInfoCard]}>Hello</div>
    </div>
  );
}

Style variations with props

You can give any component a custom prop, and reference that prop in your component styles, effectively using the prop to turn on a “flavor” of that component

For example, let’s make a styled component that acts as a card. Of course, this could be done with any AntD component, or any component at all. But we’ll do this with a humble div to illustrate the point

const SuperCard = styled.div`
  ${({ theme, cutout }) => `
    padding: ${theme.gridUnit * 2}px;
    border-radius: ${theme.borderRadius}px;
    box-shadow: 10px 5px 10px #ccc ${cutout && `inset`};
    border: 1px solid ${cutout ? transparent : theme.colors.secondary.light3 };
  `
`;

Then just use the component as <SuperCard>Some content</SuperCard> or with the (potentially dynamic) prop: <SuperCard cutout>Some content</SuperCard>

Styled component tips

No need to use theme the hard way:

It’s very tempting (and commonly done) to use the theme prop inline in the template literal like so:

const SomeStyledThing=styled.div`
  padding: ${({ theme }) => theme.gridUnit * 2}px;	
  border-radius: ${({ theme }) => theme.borderRadius}px;
  border: 1px solid ${({ theme }) => theme.colors.secondary.light3};
`;

Instead, you can make things a little easier to read/type by writing it like so:

const SomeStyledThing=styled.div`
  ${({ theme }) => `
    padding: ${theme.gridUnit * 2}px;	
    border-radius: ${theme.borderRadius}px;
    border: 1px solid ${theme.colors.secondary.light3};
  `}
`;

Extend an AntD component with custom styling

As mentioned, you want to keep your styling as close to the root of your component system as possible, to minimize repetitive styling/overrides, and err on the side of reusability. In some cases, that means you’ll want to globally tweak one of our core components to match our design system. In Superset, that’s Ant Design (AntD).

AntD uses a cool trick called compound components. For example, the Menu component also lets you use Menu.Item, Menu.SubMenu, Menu.ItemGroup, and Menu.Divider

The Object.assign trick:

Let's say you want to override an AntD component called Foo, and have Foo.Bar display some custom CSS for the Bar compound component. You can do it effectively like so:

import {
  Foo as AntdFoo,
} from 'antd';

export const StyledBar = styled(AntdFoo.Bar)`
  border-radius: ${({ theme }) => theme.borderRadius}px;
`;

export const Foo = Object.assign(AntdFoo, {
  Bar: StyledBar,
});

You can then import this customized Foo and use Foo.Bar as expected. You should probably save your creation in src/common/components for maximum reusability, and add a Storybook entry so future engineers can view your creation, and designers can better understand how it fits the Superset Design System.

@rusackas rusackas changed the title [WIP][proposal][style-guide]Tips and best practices for Emotion styled/css implementation [Proposal][style-guide]Tips and best practices for Emotion styled/css implementation Jun 22, 2021
@rusackas
Copy link
Member Author

I think this doc needs some "DOs" and "DON'Ts" before we can call it a "coding guideline" - any thoughts on the matter, anyone?

@zhaoyongjie zhaoyongjie added the sip Superset Improvement Proposal label Jun 24, 2021
@michael-s-molina
Copy link
Member

michael-s-molina commented Jun 30, 2021

Great guide @rusackas! Thanks for the hard work.

I would like to add some points extracted from CSS prop vs styled to help with the discussion.

I generally prefer the css style api for the following reasons:

You’re still writing regular React components.
Especially when working with typescript, I consider this beneficial. You type a Button component just as you would a regular React component, allowing you to clearly specify which props are accepted. As a result, you’re less likely to pollute your dom with odd attributes—a problem I found common when passing custom attributes to styled components.

Object styles are easier to work with.
When working with typescript, I love that all of my css is typechecked and provides robust autocompletion. And I generally find it easier to insert theme variables into objects instead of using the ${theme.color.red} style notation. The small downside to objects is that they are slightly more cumbersome to write and aren’t easily copied from browsers.

You colocate styles with elements.
With the css prop, what you see is what you get. It’s a small point, but not having to scroll away from an element to find your style definition really improves my workflow. It feels more efficient and keeps me in the flow when writing my components. Need to delete an element? There’s no need to hunt down the orphaned style definition.

For me, one thing that is really important is the typescript checking, and this can be achieved either by using styled or the css property because both accept a CSS-in-JS object. I also found that handling conditional styling is way easier when using the CSS-in-JS approach.

you’re less likely to pollute your dom with odd attributes

This really happens in our codebase. We have a lot of warnings of properties that are being passed to styled components without the shouldForwardProp. By the way, this is only required because the styled mixes the component's properties with properties used to conditionally style.

We also can achieve reusability with the css prop.

const StatusThing = props => (
  <div 
    css={{ padding: 10, borderRadius: 10 }} 
    {...props} 
  />
);

const InfoThing = props => (
  <StatusThing
    css={{ background: 'blue', '&::before': { content: 'i' } }}
    {...props}
  />
);
const SeparatorOnlyUsedInThisComponent = props => (
  <hr 
    css={{ 
      height: 12,
      border: 0,
      boxShadow: 'inset 0 12px 12px -12px rgba(0, 0, 0, 0.5)',
    }}
    {...props}
  />
);

Since we're using CSS-in-JS all style props are being checked by typescript and we get autocompletion while coding.

For me, the difference is bigger when we have conditional styling:

const StyledSelect = styled(AntdSelect, {
  shouldForwardProp: prop => prop !== 'hasHeader',
})<{ hasHeader: boolean }>`
  ${({ theme, hasHeader }) => `
    width: 100%;
    margin-top: ${hasHeader ? theme.gridUnit : 0}px;
  `}
`;
<Select css={{ width: '100%', marginTop: hasHeader ? theme.gridUnit : 0 }} />

Preferences aside, the real gain is defining the standard, so thanks again for this awesome guide, it will be great content for our wiki 👏🏼 🚀

@rusackas
Copy link
Member Author

rusackas commented Jul 12, 2021

Thoughts on some "rules" to make this more of a style guide. Please feel free to suggest additional ones or contest anything controversial here.

DO use styled when you want to include additional (nested) class selectors in your styles
DO use styled components when you intend to export a styled component for re-use elsewhere
DO use css when you want to amend/merge sets of styles compositionally
DO use css when you're making a small, or single-use set of styles for a component
DO move your style definitions for the css prop to an external variable when they get long
DO prefer tagged template literals (css={css...) over style objects wherever possible for maximum style portability/consistency (note: typescript support may be diminished, but IDE plugins like this make life easy)

DO NOT use styled for small, single-use style tweaks that would be easier to read/review if they were inline
DO NOT export incomplete AntD components (make sure all their compound components are exported)

@rusackas
Copy link
Member Author

rusackas commented Sep 9, 2021

Added to wiki here

@michael-s-molina michael-s-molina added the guidelines Proposals for new or updated guidelines label Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidelines Proposals for new or updated guidelines preset-io
Projects
None yet
Development

No branches or pull requests

4 participants