Creating an official octicons-react component

[jonrohan]

This Pull Requests introduces a new library for rendering Octicons in React. I started out this process by reading and following this excellent article by @necolas. There were a few things that I did not need to do because we already have the node library for loading and reading the icons.

A couple of tool choices

I tried a couple of things, first being the react-native-web suggested in the article. I ran into a few setup problems (probably my newness with react) and decided it was overkill for what I wanted to do.

The second thing I tried was using styled-components 💅 to apply the initial icon styling to each component. This didn't feel right either. I didn't want people to not be able to use this library because of the choices styled-components makes on inlining CSS.

So my approach involved inlining the styles on the <svg element using just react. I think this is the best approach because it leaves the dependencies down and doesn't interfere with someone's setup. This is the resulting dependencies ✨:

"dependencies": {
    "octicons": "7.1.0",
    "react": "^16.2.0"
}

Example

// Example usage
import Octicon from "@github/octicons-react"

const AlertIcon = <Octicon name="alert" />
// Rendered
// <svg version="1.1" width="16" height="16" viewBox="0 0 16 16" aria-hidden="true" style="display:inline-block;fill:currentColor;vertical-align:text-bottom;position:relative;user-select:none"><path fill-rule="evenodd" d="M8.865 1.52c-.18-.31-.51-.5-.87-.5s-.69.19-.87.5L.275 13.5c-.18.31-.18.69 0 1 .19.31.52.5.87.5h13.7c.36 0 .69-.19.86-.5.17-.31.18-.69.01-1L8.865 1.52zM8.995 13h-2v-2h2v2zm0-3h-2V6h2v4z"></path></svg>

--
closes primer/react#2
cc @max @broccolini for review
cc anyone who's asked me about react octicons @jglovier @jasonlong

[jonrohan]

Forgot to mention, if you want to test it, there’s an alpha release up on https://www.npmjs.org/package/%40github%2Focticons-react

[broccolini]

This looks great ⚡️

We talked over this on Zoom, here's a summary:

• agree that react for web or styled components are probably unnecessary here
• inline style for now are fine, however we may want to use a class selector in future, depending on whether folks think this is something that should be cached etc. It doesn't matter if we change that in future since the api will stay the same :)
• Ocitcons are meant to work in common sizes, it would be great to have some pre-defined size props as well as being able to pass in a specific height, such as <Octicon name='alert' small />

[broccolini]

I think showing examples where you use the icons within another other component might be clearer, this is example is kind of making a component of a component 🙃

Here's a quick example:

<ButtonDanger>
  <Octicon name='alert'>
  Delete
</ButtonDanger>

Also, pretty sure we want to be using single quotes.

[jasonlong]

Awesome! I'm using a different Octicons package for Launch so far, but I'll give this a try and let you know if I run into any problems.

[broccolini]

During our sync we discussed making the following updates:

• add props for vertical alignment with default being vertical-align: bottom
• remove position: relative

[josh]

This is really neat that we'll have an official package of this icons.

But I don't think the API as currently designed lends itself to build step optimizations to only include the necessary icons.

Also flow types would be nice to have.

[josh]

It'd be ideal to avoid dangerouslySetInnerHTML for performance reasons. When this component is marked as dirty, react will have to update the DOM using innerHTML rather than changing just the attribute that has changed.

https://medium.com/@paularmstrong/twitter-lite-and-high-performance-react-progressive-web-apps-at-scale-d28a00e780a3#3732

Since all the SVGs are static, we should be able to head of time generate the React.createElement tree builder calls.

[josh]

inline style for now are fine, however we may want to use a class selector in future, depending on whether folks think this is something that should be cached etc.

Yeah, it'd be nice if this was compatible with webpack's extract-text-webpack-plugin method of static analysis.

[josh]

I think the named import interface is going to work well—as your are moving towards.

But if we’re are going to support dynamic icon interface too, that will likely need to be a seperate file to avoid breaking tree shaking for apps using static imports. The current implementation depends on top level side effects that can’t be safely removed.

I think I’d suggest we’d not support dynamic icon names if possible. It seems like a bit of footgun to have lying around. If it’s just used once, it breaks all optimizations.

[josh]

So the presence of this internal octicons registery is going to bust tree shaking. Because mutating this object is a top level side effect, the compiler will be forced to always include all icons no matter.

[jonrohan]

😢

Ok, I've come to the realization that my dream of single imports isn't going to work. After talking through with the design-systems team, that what we want to default to is dynamic name= property. We feel like even though your use case is best practice, we want people to be able to experiment with the full library.

Once users reach a point where they're shipping their code to production, then can switch to the optimized imports and only get specific icons.

// Importing all the octicons
import Octicon from '@github/octicons-react'

// For optimizing your code and importing only specific octicons.
import {AlertOcticon, XOcticon} from '@github/octicons-react/named'

@josh @koddsson does this work for you?

[koddsson]

I don't mind the longer name for optimised imports but I'll let @josh have the final say 😸

[hiendv]

Guys, I've been publishing an octicons library with tree-shaking support for 6 months at https://github.com/hiendv/octicons-modular. I'm publishing the react component in days too. From my perspective, it's possible.

[josh]

👍 The generated source seems right. Posting here for review.

// alert.js
'use strict';

Object.defineProperty(exports, "__esModule", {
  value: true
});

var _createIconComponent = require('../utils/createIconComponent');

var _createIconComponent2 = _interopRequireDefault(_createIconComponent);

var _react = require('react');

var _react2 = _interopRequireDefault(_react);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var AlertOcticon = (0, _createIconComponent2.default)({ content: _react2.default.createElement(
    'g',
    null,
    _react2.default.createElement('path', { fillRule: 'evenodd', d: 'M8.893 1.5c-.183-.31-.52-.5-.887-.5s-.703.19-.886.5L.138 13.499a.98.98 0 0 0 0 1.001c.193.31.53.501.886.501h13.964c.367 0 .704-.19.877-.5a1.03 1.03 0 0 0 .01-1.002L8.893 1.5zm.133 11.497H6.987v-2.003h2.039v2.003zm0-3.004H6.987V5.987h2.039v4.006z' })
  ), options: { "version": "1.1", "width": 16, "height": 16, "viewBox": "0 0 16 16", "aria-hidden": "true" } });
AlertOcticon.displayName = 'AlertOcticon';
exports.default = AlertOcticon;

// createIconComponent.js
"use strict";

Object.defineProperty(exports, "__esModule", {
  value: true
});

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; };

var _react = require("react");

var _react2 = _interopRequireDefault(_react);

function _interopRequireDefault(obj) { return obj && obj.__esModule ? obj : { default: obj }; }

var createIconComponent = function createIconComponent(_ref) {
  var content = _ref.content,
      options = _ref.options;


  var prepare = function prepare(props) {
    var ariaLabel = props.ariaLabel,
        medium = props.medium,
        large = props.large;

    var attributes = Object.assign({}, options);

    // Default octicon sizes to small
    attributes["height"] = 16;

    // Read in small, medium, large props
    if (medium) {
      attributes["height"] = 32;
    } else if (large) {
      attributes["height"] = 64;
    }

    // Calculate the width based on height
    attributes["width"] = attributes["height"] * options["width"] / options["height"];

    // If the user passed in aria-label
    if (ariaLabel) {
      attributes["aria-label"] = ariaLabel;
      attributes["role"] = "img";

      // Un-hide the icon
      delete attributes["aria-hidden"];
    }

    return attributes;
  };

  var alignment = function alignment(props) {
    var top = props.top,
        middle = props.middle;


    if (top) {
      return "text-top";
    } else if (middle) {
      return "middle";
    }
    return "text-bottom";
  };

  return function (props) {
    var svgStyle = {
      display: "inline-block",
      fill: "currentColor",
      verticalAlign: alignment(props),
      userSelect: "none"
    };

    return _react2.default.createElement(
      "svg",
      _extends({}, prepare(props), {
        style: svgStyle }),
      content
    );
  };
};

exports.default = createIconComponent;

The nice thing about this dependency chain is that each additional icon will only incur a small marginal cost after the shared createIconComponent is included in the bundle.

[hiendv]

IMO, the react/vuejs component should only wrap the props and pass them straight to the octicons and render the HTML.

import { zap } from "@github/octicons"
import Octicon from "@github/octicons-react"

<Octicon icon={zap} />

[colebemis]

This looks great! I've been thinking about doing something very similar with Feather. I really like the named import approach. It seems like an elegant way to minimize bundle size.

Thank you for all the work you and your team have put into this project! It's been a huge inspiration for Feather 🙂

[shawnbot]

Closing in favor of #222! ❤️