primer / octicons

A scalable set of icons handcrafted with <3 by GitHub
https://primer.style/foundations/icons
MIT License
8.32k stars 827 forks source link

Refactor octicons_react to use API from octicons_node #250

Closed jonrohan closed 3 years ago

jonrohan commented 6 years ago

Making a note that we should refactor out the svg calculation work from octicons_react and use the octicons_node api. This will allow us to unit test everything on octicons_node and only need to test if octicons_react compiles react. Catching bugs like #249

I think it's not a perfect fit at the moment, but adding some endpoints for getting json prop values would be ideal.

shawnbot commented 6 years ago

I was going to object to this because I thought that this might make the the bundle much larger, but after looking at octicons vs octicons-react sizes, I think I'm actually kind of into it:

Name Type Size
octicons
index.js application/javascript 1.96 kB
build/data.json application/json 84.03 kB
octicons-react
index.esm.js application/javascript 78.3 kB
index.umd.js application/javascript 84.57 kB

In order to keep the React build slim we'd need to remove the SVG path strings from data.json, but we could do that as part of the install/prepare/build. Maybe it looks something like this, so that we can pluck the attribute getter from octicons without bringing along the data and calling it with metadata on each icon function (which is where we're currently stashing the size):

// octicons_node/attrs.js
module.exports = function getAttributesFactory(icon) {
  const attrs = {
    version: "1.1",
    width: icon.width,
    height: icon.height,
    viewBox: [0, 0, icon.width, icon.height].join(' '),
    "class": `octicon octicon-${icon.name}`,
    "aria-hidden": "true"
  }
  return function getAttributes(options) {
    // this is the body of htmlAttributes() in octicons_node/index.js
  }
}

// octicons_node/index.js
const icons = require('./build/data.json')
const getAttributesFactory = require('./attrs')
module.exports = Object.keys(icons).reduce((octicons, key) => {
  const icon = octicons[key] = icons[key]
  const getAttributes = getAttributesFactory(icon)
  // ...
  octicons[key].toSVG = options => {
    return '<svg ' + getAttributes(options) + '...' // etc.
  }
  return octicons
}, {})

// octicons_react/index.js
import getAttributesFactory from 'octicons/attrs'
export default function Octicon({icon: Icon, children, ...rest}) {
  // Icon.meta is generated at build time; these getters could be cached
  const props = getAttributesFactory(Icon.meta)(rest)
  return <svg {...props}>{Icon ? <Icon /> : children}</svg>
}
github-actions[bot] commented 3 years ago

Hi! This issue has been marked as stale because it has been open with no activity for 180 days. You can comment on the issue or remove the stale label to keep it open. If you do nothing, this issue will be closed in 7 days.