GoogleForCreators / web-stories-wp

Web Stories for WordPress
https://wp.stories.google
Apache License 2.0
768 stars 178 forks source link

Consider using SVG sprites for icons #13411

Open swissspidy opened 1 year ago

swissspidy commented 1 year ago

Task Description

Stumbling upon https://kurtextrem.de/posts/svg-in-js made me think about using SVG sprites again to improve our bundle size. Would be an interesting exploration if it could be automated as much as possible.

swissspidy commented 4 months ago

@Swanand01 This would be a performance ticket where we might see a more impressive performance win in the browser, as we have a lot of SVG icons. We'd need a way to keep our icons as SVG files, and then there might be a build step involved to create the sprite. This would require some changes to webpack etc. so it's not trivial.

Maybe this is something you'd be interested to look into a little bit? I'd just recommend timeboxing it to a few hours as it's a bit of a rabbit hole. I'm curious to learn what is possible and how much effort it is before going all-in on implementation.

Swanand01 commented 4 months ago

I found this Webpack loader that creates SVG sprites and renders them using the <use> tag.

import twitterLogo from './logos/twitter.svg';
// twitterLogo === SpriteSymbol<id: string, viewBox: string, content: string>
// Extract mode: SpriteSymbol<id: string, viewBox: string, url: string, toString: Function>

const rendered = `
<svg viewBox="${twitterLogo.viewBox}">
  <use xlink:href="#${twitterLogo.id}" />
</svg>`;

This way we can have our icons as SVG files, and won't have to change how we're rendering them

swissspidy commented 4 months ago

Hmm I might have seen that one in the past. Looks promising on the surface, even though it hasn't been updated in years. Does it still work?

Swanand01 commented 4 months ago

Tried this in the Webpack config:

{
            issuer: /\.[jt]sx?$/,
            include: [/\/icons\/.*\.svg$/],
            use: [
              { loader: 'svg-sprite-loader' },
              {
                loader: '@svgr/webpack',
                options: {
                  titleProp: true,
                  svgo: true,
                  memo: true,
                  svgoConfig: {
                    plugins: [
                      {
                        name: 'preset-default',
                        params: {
                          overrides: {
                            removeViewBox: false,
                            convertColors: {
                              currentColor: /^(?!url|none)/i,
                            },
                          },
                        },
                      },
                      'removeDimensions',
                    ],
                  },
                },
              },
            ],
          },

Compiles with this error for a lot of images:

ERROR in ./packages/design-system/src/icons/trash.svg
Module build failed (from ./node_modules/svg-sprite-loader/lib/loader.js):
InvalidSvg: svg-sprite-loader exception.
swissspidy commented 4 months ago

I don't think it can be used together with @svgr/webpack. SVGR turns SVG files into React components, and you can't pass a React component to svg-sprite-loader, as it expects the raw SVG.

We would need to remove @svgr/webpack from the list. Instead, we should use svgo-loader though (which would otherwise be set up by SVGR) , which optimizes SVGs on the fly before including them. Just like the svg-sprite-loader examples do it:

https://github.com/JetBrains/svg-sprite-loader/blob/c760cf93c49736f1e26a85fff7db3f9c940572c3/examples/browser-sprite/webpack.config.js

Swanand01 commented 4 months ago

This did compile successfully, but now there's a problem with styled-components:

Cannot create styled-component for component: <symbol xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" id="launch"><path fill="none" d="M0 0h24v24H0z" /><path d="M19 19H5V5h7V3H5a2 2 0 0 0-2 2v14a2 2 0 0 0 2 2h14c1.1 0 2-.9 2-2v-7h-2zM14 3v2h3.59l-9.83 9.83 1.41 1.41L19 6.41V10h2V3z" /></symbol>

This occurred in packages/element-library/src/video/playPauseButton.js

const Play = styled(Icons.PlayFilled)`
  ${iconCss};
`;
const Pause = styled(Icons.StopFilled)`
  ${iconCss};
`;

Maybe we have to find a different way to apply CSS.

Edit: The loader transforms the imported svg file into<symbol>, and not <svg>, thus the error. I suppose some more configuration is needed to extract the actual <svg>

swissspidy commented 4 months ago

Maybe we have to find a different way to apply CSS.

Correct.

So the svg sprite contains a <def> with a <symbol> for each icon. In the place where we actually want to use the icon, we need <svg><use xlink:href="#nameoftheicon"></use></svg>

Looking at the svg-sprite-loader examples, it would be probably require something like this (untested):

const SVG = styled.svg`
  ${iconCss};
`;
const Play = () => <SVG viewBox=`${Icons.PlayFilled.viewBox}`>
  <use xlink:href=`#${Icons.PlayFilled.id}`></use>
</SVG>;
const Pause = () => <SVG viewBox=`${Icons.StopFilled.viewBox}`>
  <use xlink:href=`#${Icons.StopFilled.id}`></use>
</SVG>;

// Rest stays as-is.

Soo... sounds like a lot of work to me :)

Swanand01 commented 4 months ago

Maybe we have to find a different way to apply CSS.

Correct.

So the svg sprite contains a <def> with a <symbol> for each icon. In the place where we actually want to use the icon, we need <svg><use xlink:href="#nameoftheicon"></use></svg>

Looking at the svg-sprite-loader examples, it would be probably require something like this (untested):

const SVG = styled.svg`
  ${iconCss};
`;
const Play = () => <SVG viewBox=`${Icons.PlayFilled.viewBox}`>
  <use xlink:href=`#${Icons.PlayFilled.id}`></use>
</SVG>;
const Pause = () => <SVG viewBox=`${Icons.StopFilled.viewBox}`>
  <use xlink:href=`#${Icons.StopFilled.id}`></use>
</SVG>;

// Rest stays as-is.

Soo... sounds like a lot of work to me :)

I suppose we'll have to do this everywhere an SVG image is used?

In that case, should we create an Icon component like this:

const Icon = ({id, className}) => (
  <svg><use xlink:href={`#${id}`} className={className} /></svg>
);

and use it everywhere an SVG image is used?

swissspidy commented 4 months ago

Looks like that project supports doing that with a custom config option:

https://github.com/JetBrains/svg-sprite-loader/tree/c760cf93c49736f1e26a85fff7db3f9c940572c3/examples/custom-runtime-generator

https://github.com/JetBrains/svg-sprite-loader/blob/c760cf93c49736f1e26a85fff7db3f9c940572c3/examples/custom-runtime-generator/main.jsx

That way we wouldn't have to change anything at all.

But that requires some testing and tinkering to see how feasible it is.