Use inline SVGs for icons

[pugnascotia]

This is an experiment in rendering icons through inline SVGs. This removes the need for consumers of EUI to also support automatic SVG sprite handling. A Babel plugin transforms the SVGs into React components automatically. Class names are still propagated, but <title> is missing. More work would be needed to make this a complete swap.

CC @cjcenizal for feedback.

[bevacqua]

Anything that simplifies the build for consumers is a +1 in my ledger

[cjcenizal]

@pugnascotia This is awesome man! I tested it with Kibana the icons show up now, though it looks like the majority of them contain errors. I wonder if the format of the SVGs themselves could be the cause? @snide What do you think? Here are the icons side by side in Kibana and in the EUI docs:

There are errors in both, but not always the same kinds of errors. For example, faceHappy and faceSad are fine in EUI but broken in Kibana. I'm not sure what to make of this discrepancy.

[snide]

Face Happy and Sad use strokes. They need to be cleaned up in EUI as well (notice that they'll fail in the dark theme). The rest of our icons though should be pretty clean and only use shapes so I don't know why you'd run into errors. Likely they aren't being translated properly.

[pugnascotia]

I've changed approach and generated React components via script and versioned them. It occurred to me that if someone wanted to consume the components as ES6, without any Babel stuff, they'd be stuffed.

In an ideal world the transform would probably still be a Babel transform of some kind, but my Babel-fu is lacking.

Thoughts?

[bevacqua]

To me it sounds like what we need to be doing, instead of this PR, is fix our build process. Can't we get rid of lib/ and instead compile our bundle with React libraries as external dependencies?

[pugnascotia]

How would that resolve the need for the SVG sprite plugin stuff?

[bevacqua]

We'd have a bundle at our interface that's already compiled, so things like SVG sprites would've been inlined into that bundle already at the boundary.

[pugnascotia]

I did try that, in Berlin I think it was, but it meant having React available as a global library - as in, on window. Maybe there's another way to inject it into a bundle, but I'm not aware of it. That mechanism is intended, AfAIK, for when you're pulling in libraries from CDNs.

Also, if we ditch the sprites method, we still need a way of being able to dynamically set the <title> in the SVG for a11y reasons, and I haven't found a transform yet that can add this.

[bevacqua]

Right. I'm saying there should be a better way of compiling than what we're doing. Isn't there a way to tell webpack not to follow import links and thus just run the plugins without bundling?

[pugnascotia]

We are already using webpack externals to exclude React and PropTypes from our bundle in dist/. If you look at the bundle's source, here's the bundled version of React:

/* 0 */
/***/ (function(module, exports) {

module.exports = window.React;

/***/ }),

We can make Cloud UI import React from window (Webpack docs). But now instead of mandating that consumers use webpack with the svg sprite plugin, we're mandating that consumers have React on the global scope. I was trying to get away from making consumers have to do anything special.

[bevacqua]

What I'm saying is doing something like this for the /lib export:

https://www.npmjs.com/package/webpack-node-externals

So that just like with dist/ not including React, we don't bundle it either, but end up with lib/index.js that looks like [very abridged example]

const React = require('react')
const inlineSvg = '...'
const anotherInlineSvg = '...'

/* ... */

module.exports = Eui

Then any consumer can just import this and it'll just work, no window bridge or webpack craze necessary.

[pugnascotia]

The files in the /lib export already have require('react') as they're generated through Babel, not webpack. There's no bundling going on there at all, just code transformation.

The first commit on this PR added babel-plugin-inline-react-svg, which kind-of did what I think you're describing, only it generated functional components that return the SVG. I moved away from that, basically because I couldn't work out how to change the plugin to inject a props-controlled <title> in the SVG, which we need for accessibility.

[bevacqua]

Maybe we can just fork the Babel plugin?

[pugnascotia]

I would happily, if I knew how to do it. I literally do not currently know enough about Babel and transforms (specifically to JSX ASTs) to be able to do the work. Conceptually it's not difficult. You first, @bevacqua :-)

[bevacqua]

Why don't we just add those <title> tags to the SVG files? Sounds like we could use a quick script to add them to the files we already have themselves, and then the plugin wouldn't need to be forked just to add a title?

[cjcenizal]

If the <title> tags are the only thing causing a problem then I think we can ditch them altogether. We originally wanted to include them to make icons accessible to screen readers, but we can accomplish the same thing by adding aria-hidden="true" to the SVG and supplying an aria-label to the wrapping element. CC'ing @timroes for his input on this.

// For an icon-only button
<EuiButton aria-label="Copy document">
  <EuiIcon type="copy" aria-hidden="true" />
</EuiButton>

// Or for an icon that represents status in a table row
<EuiIcon type="check" aria-label="Successfully saved" />

I can't think of any situation where we'll be using an icon without some wrapping element or some supplementary element which we can use to provide information in this manner.

[pugnascotia]

Oh for real? Man, I wish I'd known that earlier.

[cjcenizal]

Sorry 😞

[timroes]

Actually in my opinion, @cjcenizal suggestion is even better than providing a title for the SVG.

As he said, we most often have the icon in buttons/links. If it's the only content, you can easily replace as shown above with an aria-label on the button. In this case the screen reader would state something like Copy document, button. If you would use a title on the SVG instead, depending on the screen reader, it might state something like: Copy document, image, button or even button, 1 child element - and you require a specific shortcut before it will read out Copy document, image.

So if the image is the only content within a button/link, better use @cjcenizal approach, then SVG title.

In cases where we use the Icon as a real image to identify something, title and aria-label both would be fine. But it should still be possible to use aria-label on the <EuiIcon> directly in that case.

[cjcenizal]

Thanks @timroes! I was the one who introduced the <title> element originally, based on suggestions from our third-party auditors. Good to know we can ditch it.

[pugnascotia]

Right, armed with new information from @cjcenizal and @timroes, we're back to the Babel plugin implementation, and it's pretty simple. Some eyes please?

[pugnascotia]

This line was duplicated.

[cjcenizal]

Can we pin this?

[cjcenizal]

This is badass! I love how much simpler this just became. I tested it with Kibana and almost everything appears fine:

Kibana on left, EUI docs on right. Only "dot", "faceHappy", and "faceSad" appear to have errors. What an odd coincidence that they all appear on the same row...

[cjcenizal]

I think we can delete aria-label={titleText} here and just expect consumers to pass in a arial-label prop directly. We'd just have to document this as a breaking change.

[pugnascotia]

I've pinned the plugin version and remove the default title.

@cjcenizal The difference in Kibana is weird. Is the rendered SVG different? Is there extra CSS being applied in Kibana? Ping me when you come online and we'll try to get to the bottom of it. If there's an easy way to get it running locally, let me know.

[pugnascotia]

@snide @bevacqua what do you think about merging this change as-is? I'm happy to work with @cjcenizal to resolve any Kibana issues if they arise.

[cjcenizal]

SGTM @pugnascotia!

[snide]

Did a visual check and some tests of usage. Seems to still work as intended. Looks good to me.