Create @primer/styled-octicons package

[colebemis]

Problem

We want to be able to style Octicons with system props like we style other Primer Components. The components in @primer/octicons-react, however, don't accept system props. So we created a StyledOcticon component in Primer Components as a workaround. But the API for using StyledOcticon is clunky:

<AlertIcon /> // how you use an icon if you don't need system props
<StyledOcticon icon={AlertIcon} mr={2} /> // to use system props, you need to wrap the icon in StyledOcticon

Solution

What if we could pass system props directly to icon components?

- <StyledOcticon icon={AlertIcon} mr={2} />
+ <AlertIcon mr={2} />

This PR makes that possible. It adds a @primer/styled-octicons package that wraps the components in @primer/octicons-react with COMMON system props.

import {AlertIcon} from '@primer/styled-octicons'

function Example () {
  return <AlertIcon mr={2} color="red.6" />
}

📝 Preview the docs

You can test out the package by installing:

npm install @primer/styled-octicons@canary

After we merge this PR, we'll be able to deprecate StyledOcticon in Primer Components.

Questions

• Should icon components accept the display prop?
  • I assume the answer is "yes" because display is part of the COMMON system props in Primer Components.
  • If the answer is "yes," we'll have to update the @primer/octicons-react because it uses inlines styles to set display: inline-block, which overrides the display prop. But how will we apply styles in @primer/octicons-react without using inline styles or styled components?
• Should icon components accept the sx prop?
• Are we happy with the name @primer/styled-octicons for this package?
• What should the peer dependencies of this package be? I've set react and styled-components as peer dependencies. But should styled-system also be a peer dependency?

TODO

• Write PR description
• Write docs
• Fix tree shaking
  • I tested the canary version of this package in @dmarcey's tree-shaking-test repo and found that tree shaking was not working. I need to figure out tree shaking works for @primer/octicons-react but not for @primer/styled-octicons.
• Add sx prop

---

Part of primer/react#511

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/primer/octicons/f0xn0lnx5
✅ Preview: https://octicons-git-styled-octicons.primer.now.sh

[emplums]

Should icon components accept the display prop?

Honestly I don't think this is a hard requirement. If Octicons are always meant to be displayed inline-block we can leave it that way.

Should icon components accept the sx prop?

yes this would be nice to have. though if we wanted to have a default theme for these icons we'd have to have a dep on @primer/components which we might not want to do

Are we happy with the name @primer/styled-octicons for this package?

yes 🚢 it!

What should the peer dependencies of this package be? I've set react and styled-components as peer dependencies. But should styled-system also be a peer dependency?

I think just react and styled-components should be fine but not sure about styled-system

[emplums]

Ok just looked through the code and it got me thinking more about the theme. Should we assume that folks are using this library inside of a ThemeProvider somewhere that's hooked up to the Primer Component's theme? Should we supply a backup theme just in case like we do in Primer Components?

[colebemis]

@emplums I think styled-system has a default spacing scale. So <AlertIcon mr={2} /> will work even outside of a ThemeProvider. I'm okay with requiring a ThemeProvider to use colors. I think that's a better option than adding @primer/components as a dependency in order to set a default theme.

[dmarcey]

@colebemis I can confirm I saw the same results as you with respect to tree-shaking. I played around with the package directly in my node_modules folder and I think I got something working when I moved the individual icons into their own files and then just exported them all from the root.

From this comment (webpack/webpack#9337 (comment)) it seems like maybe that's the best way to handle things anyway? I'm guessing since we're now executing a function call each icon (to compose it with system props), as opposed to just defining in the functions like in the octicons-v2-react package, the optimizer doesn't know what to omit.

However, it can use more of a "side-effects" approach which each icon is in its own module, perhaps?

Feel free to ping me if you want to talk through some of this synchronously and I can walk you through what I tried.

[colebemis]

🎉 Bundling each icon individually fixed the tree-shaking issue! Thanks for your help, @dmarcey.

[colebemis]

@dmarcey @emplums This is ready for final review 😸

[dmarcey]

This looks great! ✨
Thanks for taking the time to get the tree-shaking working!

One thought I had for a future follow-up was to try to find a way to pull some of the common code out into a separate module that can be imported by each individual icon.

import { color, compose, space } from 'styled-system';
var sizeMap = {
    small: 16,
    medium: 32,
    large: 64
  };
  
 export function getSvgProps(_ref) {
    var ariaLabel = _ref['aria-label'],
        className = _ref.className,
        size = _ref.size,
        verticalAlign = _ref.verticalAlign,
        svgDataByHeight = _ref.svgDataByHeight;
  
    var height = sizeMap[size] || size;
    var naturalHeight = closestNaturalHeight(Object.keys(svgDataByHeight), height);
    var naturalWidth = svgDataByHeight[naturalHeight].width;
    var width = height * (naturalWidth / naturalHeight);
    var path = svgDataByHeight[naturalHeight].path;
  
    return {
      'aria-hidden': ariaLabel ? 'false' : 'true',
      'aria-label': ariaLabel,
      role: 'img',
      className: className,
      viewBox: '0 0 ' + naturalWidth + ' ' + naturalHeight,
      width: width,
      height: height,
      fill: 'currentColor',
      style: {
        display: 'inline-block',
        userSelect: 'none',
        verticalAlign: verticalAlign
      },
      dangerouslySetInnerHTML: { __html: path }
    };
  }
  
  function closestNaturalHeight(naturalHeights, height) {
    return naturalHeights.map(function (naturalHeight) {
      return parseInt(naturalHeight, 10);
    }).reduce(function (acc, naturalHeight) {
      return naturalHeight <= height ? naturalHeight : acc;
    }, naturalHeights[0]);
  }
  
 export var _extends = Object.assign || function (target) {
    for (var i = 1; i < arguments.length; i++) {
      var source = arguments[i];for (var key in source) {
        if (Object.prototype.hasOwnProperty.call(source, key)) {
          target[key] = source[key];
        }
      }
    }return target;
  };

  // eslint-disable-next-line import/no-namespace

export var COMMON = compose(space, color);

Those lines are duplicated across each *Icon.js so when the consumer ends up bundling the package, they get a copy of those functions for each different type of icon they pull in - it looks like its about 500 bytes minified or so per icon. So making a change like this could save 10 KB for an app that uses 20 icons.

Just a suggestion for a follow-up, I think this is good to 🚢 as is!

[emplums]

Why do we recommend folks to install the canary?

[colebemis]

Until the v2 branch is merged, there won't actually be a @latest version of the package. When the v2 branch is merged, we'll update the docs to remove @canary.

[emplums]

🙌