web-stories-wp icon indicating copy to clipboard operation
web-stories-wp copied to clipboard

Published 20 hours ago • verified publisher icon GoogleForCreators

Use SVG sprites for icons

Open Swanand01 opened this issue 1 year ago • 15 comments

Summary

This PR adds support for using an svg sprite to render icons. The svg-sprite-loader webpack loader is used to generate the sprites.

User-facing changes

None.

Testing Instructions

This PR can be tested by following these steps:

  1. Inspect any svg icon in the plugin.
  2. You should see markup like this:
<svg viewBox="0 0 24 24" width="22px">
  <use xlink:href="#homeWithHeart"></use>
</svg>

Reviews

Does this PR have a security-related impact?

No.

Does this PR change what data or activity we track or use?

No.

Does this PR have a legal-related impact?

No.

Checklist

  • [x] This PR addresses an existing issue and I have linked this PR to it
  • [x] I have tested this code to the best of my abilities
  • [x] I have verified accessibility to the best of my abilities (docs)
  • [ ] I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • [x] This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • [x] I have added documentation where necessary
  • [x] I have added a matching Type: XYZ label to the PR

Fixes #13411

Swanand01 avatar Jun 19 '24 04:06 Swanand01

Hi @swissspidy, following our discussion from #13411, we implemented a custom runtime generator as shown in the svg-sprite-loader examples. However, we now seem to be running into this:

ERROR in ./packages/design-system/src/icons/align_middle.svg 17:13
Module parse failed: Unexpected token (17:13)
File was processed with these loaders:
 * ./node_modules/svg-sprite-loader/lib/loader.js
 * ./node_modules/svgo-loader/index.js
You may need an additional loader to handle the result of these loaders.
|
|     export default function AlignMiddleSpriteSymbolComponent (props) {
>       return <SpriteSymbolComponent glyph="align_middle" {...props} />;
|     }
|
 @ ./packages/design-system/src/icons/index.ts 26:0-60 26:0-60
 @ ./packages/design-system/src/index.ts 29:0-33 33:0-17
 @ ./packages/wp-dashboard/src/index.js 39:0-65 70:2-15

Swanand01 avatar Jun 19 '24 04:06 Swanand01

Need to add babel-loader as well to parse the JSX, as per the example. https://github.com/JetBrains/svg-sprite-loader/blob/master/examples/custom-runtime-generator/webpack.config.js

swissspidy avatar Jun 19 '24 05:06 swissspidy

The icons now appear 🎉 But they are way too big, should we add CSS for the icon class added to the icon.jsx component?

Swanand01 avatar Jun 19 '24 06:06 Swanand01

Sounds like progress!

We don‘t really use CSS classes as we use styled-components. Perhaps we can do something like applying viewBox from the symbol to the svg in the runtime or so. Needs some exploration/testing.

swissspidy avatar Jun 19 '24 07:06 swissspidy

Sounds like progress!

We don‘t really use CSS classes as we use styled-components. Perhaps we can do something like applying viewBox from the symbol to the svg in the runtime or so. Needs some exploration/testing.

Yes, applying the viewBox fixed it! Thanks a lot 🎉

Swanand01 avatar Jun 19 '24 07:06 Swanand01

All checks passing 🎉 I'll move the Icon component to the design-system package and add some tests for it. The PR can be marked ready for review post that.

Swanand01 avatar Jun 19 '24 09:06 Swanand01

Size Change: +17.6 kB (+0.64%)

Total Size: 2.77 MB

Filename Size Change
assets/css/web-stories-block-rtl.css 5.76 kB +1.11 kB (+23.86%) 🚨
assets/css/web-stories-block.css 5.8 kB +1.11 kB (+23.75%) 🚨
assets/css/web-stories-list-styles-rtl.css 3.49 kB +1.06 kB (+43.86%) 🚨
assets/css/web-stories-list-styles.css 3.52 kB +1.06 kB (+43.29%) 🚨
assets/js/7830.js 0 B -38.2 kB (removed) 🏆
assets/js/9947.js 0 B -97.6 kB (removed) 🏆
assets/js/web-stories-block.js 31.2 kB +3.68 kB (+13.4%) ⚠️
assets/js/web-stories-dashboard.js 64.1 kB +514 B (+0.81%)
assets/js/web-stories-editor.js 1.46 MB +5.42 kB (+0.37%)
assets/js/1918.js 38.7 kB +38.7 kB (new file) 🆕
assets/js/5803.js 101 kB +101 kB (new file) 🆕
ℹ️ View Unchanged
Filename Size Change
assets/css/web-stories-carousel-rtl.css 711 B 0 B
assets/css/web-stories-carousel.css 711 B 0 B
assets/css/web-stories-dashboard-rtl.css 657 B 0 B
assets/css/web-stories-dashboard.css 659 B 0 B
assets/css/web-stories-editor-rtl.css 767 B 0 B
assets/css/web-stories-editor.css 769 B 0 B
assets/css/web-stories-embed-rtl.css 662 B 0 B
assets/css/web-stories-embed.css 664 B 0 B
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B 0 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B 0 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 253 B 0 B
assets/css/web-stories-theme-style-twentyfifteen.css 253 B 0 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 286 B 0 B
assets/css/web-stories-theme-style-twentyfourteen.css 286 B 0 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 310 B 0 B
assets/css/web-stories-theme-style-twentyseventeen.css 310 B 0 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 241 B 0 B
assets/css/web-stories-theme-style-twentysixteen.css 241 B 0 B
assets/css/web-stories-theme-style-twentyten-rtl.css 142 B 0 B
assets/css/web-stories-theme-style-twentyten.css 142 B 0 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 265 B 0 B
assets/css/web-stories-theme-style-twentytwelve.css 265 B 0 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B 0 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 324 B 0 B
assets/css/web-stories-theme-style-twentytwentyone.css 324 B 0 B
assets/css/web-stories-widget-rtl.css 459 B 0 B
assets/css/web-stories-widget.css 459 B 0 B
assets/js/3768.js 13.9 kB 0 B
assets/js/3933.js 27.3 kB 0 B
assets/js/4032.js 4.74 kB 0 B
assets/js/5380.js 8.18 kB 0 B
assets/js/911.js 218 kB 0 B
assets/js/9391.js 95 B 0 B
assets/js/945.js 49.5 kB 0 B
assets/js/chunk-colorthief.js 2.63 kB 0 B
assets/js/chunk-ffmpeg.js 5.98 kB 0 B
assets/js/chunk-html-to-image.js 4.51 kB 0 B
assets/js/chunk-media-gallery.js 6.12 kB -3 B (-0.05%)
assets/js/chunk-mediainfo.js 95 B 0 B
assets/js/chunk-opentype.js 97 B 0 B
assets/js/chunk-react-calendar.js 10.9 kB 0 B
assets/js/chunk-react-color.js 25.9 kB 0 B
assets/js/chunk-selfie-segmentation.js 16.3 kB 0 B
assets/js/chunk-web-stories-template-0-metaData.js 546 B 0 B
assets/js/chunk-web-stories-template-0.js 10.8 kB 0 B
assets/js/chunk-web-stories-template-1-metaData.js 537 B 0 B
assets/js/chunk-web-stories-template-1.js 9.16 kB 0 B
assets/js/chunk-web-stories-template-10-metaData.js 531 B 0 B
assets/js/chunk-web-stories-template-10.js 7.18 kB 0 B
assets/js/chunk-web-stories-template-11-metaData.js 536 B 0 B
assets/js/chunk-web-stories-template-11.js 8.77 kB 0 B
assets/js/chunk-web-stories-template-12-metaData.js 494 B 0 B
assets/js/chunk-web-stories-template-12.js 8.55 kB 0 B
assets/js/chunk-web-stories-template-13-metaData.js 521 B 0 B
assets/js/chunk-web-stories-template-13.js 6.84 kB 0 B
assets/js/chunk-web-stories-template-14-metaData.js 580 B 0 B
assets/js/chunk-web-stories-template-14.js 7.2 kB 0 B
assets/js/chunk-web-stories-template-15-metaData.js 541 B 0 B
assets/js/chunk-web-stories-template-15.js 8.68 kB 0 B
assets/js/chunk-web-stories-template-16-metaData.js 586 B 0 B
assets/js/chunk-web-stories-template-16.js 10.4 kB 0 B
assets/js/chunk-web-stories-template-17-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-17.js 8.96 kB 0 B
assets/js/chunk-web-stories-template-18-metaData.js 582 B 0 B
assets/js/chunk-web-stories-template-18.js 9.19 kB 0 B
assets/js/chunk-web-stories-template-19-metaData.js 501 B 0 B
assets/js/chunk-web-stories-template-19.js 9.05 kB 0 B
assets/js/chunk-web-stories-template-2-metaData.js 584 B 0 B
assets/js/chunk-web-stories-template-2.js 8.93 kB 0 B
assets/js/chunk-web-stories-template-20-metaData.js 548 B 0 B
assets/js/chunk-web-stories-template-20.js 8.61 kB 0 B
assets/js/chunk-web-stories-template-21-metaData.js 536 B 0 B
assets/js/chunk-web-stories-template-21.js 9.36 kB 0 B
assets/js/chunk-web-stories-template-22-metaData.js 523 B 0 B
assets/js/chunk-web-stories-template-22.js 7.37 kB 0 B
assets/js/chunk-web-stories-template-23-metaData.js 600 B 0 B
assets/js/chunk-web-stories-template-23.js 6.9 kB 0 B
assets/js/chunk-web-stories-template-24-metaData.js 516 B 0 B
assets/js/chunk-web-stories-template-24.js 11.1 kB 0 B
assets/js/chunk-web-stories-template-25-metaData.js 541 B 0 B
assets/js/chunk-web-stories-template-25.js 6.74 kB 0 B
assets/js/chunk-web-stories-template-26-metaData.js 598 B 0 B
assets/js/chunk-web-stories-template-26.js 6.89 kB 0 B
assets/js/chunk-web-stories-template-27-metaData.js 541 B 0 B
assets/js/chunk-web-stories-template-27.js 7.6 kB 0 B
assets/js/chunk-web-stories-template-28-metaData.js 532 B 0 B
assets/js/chunk-web-stories-template-28.js 8.69 kB 0 B
assets/js/chunk-web-stories-template-29-metaData.js 561 B 0 B
assets/js/chunk-web-stories-template-29.js 8.84 kB 0 B
assets/js/chunk-web-stories-template-3-metaData.js 533 B 0 B
assets/js/chunk-web-stories-template-3.js 8.16 kB 0 B
assets/js/chunk-web-stories-template-30-metaData.js 574 B 0 B
assets/js/chunk-web-stories-template-30.js 7.35 kB 0 B
assets/js/chunk-web-stories-template-31-metaData.js 503 B 0 B
assets/js/chunk-web-stories-template-31.js 9.83 kB 0 B
assets/js/chunk-web-stories-template-32-metaData.js 551 B 0 B
assets/js/chunk-web-stories-template-32.js 12.1 kB 0 B
assets/js/chunk-web-stories-template-33-metaData.js 491 B 0 B
assets/js/chunk-web-stories-template-33.js 8.83 kB 0 B
assets/js/chunk-web-stories-template-34-metaData.js 570 B 0 B
assets/js/chunk-web-stories-template-34.js 7.35 kB 0 B
assets/js/chunk-web-stories-template-35-metaData.js 565 B 0 B
assets/js/chunk-web-stories-template-35.js 8.61 kB 0 B
assets/js/chunk-web-stories-template-36-metaData.js 574 B 0 B
assets/js/chunk-web-stories-template-36.js 11.9 kB 0 B
assets/js/chunk-web-stories-template-37-metaData.js 528 B 0 B
assets/js/chunk-web-stories-template-37.js 6.09 kB 0 B
assets/js/chunk-web-stories-template-38-metaData.js 568 B 0 B
assets/js/chunk-web-stories-template-38.js 7.55 kB 0 B
assets/js/chunk-web-stories-template-39-metaData.js 586 B 0 B
assets/js/chunk-web-stories-template-39.js 7.7 kB 0 B
assets/js/chunk-web-stories-template-4-metaData.js 562 B 0 B
assets/js/chunk-web-stories-template-4.js 11.6 kB 0 B
assets/js/chunk-web-stories-template-40-metaData.js 557 B 0 B
assets/js/chunk-web-stories-template-40.js 9.73 kB 0 B
assets/js/chunk-web-stories-template-41-metaData.js 569 B 0 B
assets/js/chunk-web-stories-template-41.js 7.39 kB 0 B
assets/js/chunk-web-stories-template-42-metaData.js 521 B 0 B
assets/js/chunk-web-stories-template-42.js 6.79 kB 0 B
assets/js/chunk-web-stories-template-43-metaData.js 555 B 0 B
assets/js/chunk-web-stories-template-43.js 8.41 kB 0 B
assets/js/chunk-web-stories-template-44-metaData.js 579 B 0 B
assets/js/chunk-web-stories-template-44.js 10.6 kB 0 B
assets/js/chunk-web-stories-template-45-metaData.js 566 B 0 B
assets/js/chunk-web-stories-template-45.js 7.09 kB 0 B
assets/js/chunk-web-stories-template-46-metaData.js 532 B 0 B
assets/js/chunk-web-stories-template-46.js 5.08 kB 0 B
assets/js/chunk-web-stories-template-47-metaData.js 589 B 0 B
assets/js/chunk-web-stories-template-47.js 8.84 kB 0 B
assets/js/chunk-web-stories-template-48-metaData.js 554 B 0 B
assets/js/chunk-web-stories-template-48.js 8.68 kB 0 B
assets/js/chunk-web-stories-template-49-metaData.js 515 B 0 B
assets/js/chunk-web-stories-template-49.js 8.5 kB 0 B
assets/js/chunk-web-stories-template-5-metaData.js 557 B 0 B
assets/js/chunk-web-stories-template-5.js 9.43 kB 0 B
assets/js/chunk-web-stories-template-50-metaData.js 504 B 0 B
assets/js/chunk-web-stories-template-50.js 8.83 kB 0 B
assets/js/chunk-web-stories-template-51-metaData.js 527 B 0 B
assets/js/chunk-web-stories-template-51.js 9.92 kB 0 B
assets/js/chunk-web-stories-template-52-metaData.js 603 B 0 B
assets/js/chunk-web-stories-template-52.js 9.89 kB 0 B
assets/js/chunk-web-stories-template-53-metaData.js 553 B 0 B
assets/js/chunk-web-stories-template-53.js 5.61 kB 0 B
assets/js/chunk-web-stories-template-54-metaData.js 544 B 0 B
assets/js/chunk-web-stories-template-54.js 7.4 kB 0 B
assets/js/chunk-web-stories-template-55-metaData.js 574 B 0 B
assets/js/chunk-web-stories-template-55.js 6.88 kB 0 B
assets/js/chunk-web-stories-template-56-metaData.js 542 B 0 B
assets/js/chunk-web-stories-template-56.js 9.45 kB 0 B
assets/js/chunk-web-stories-template-57-metaData.js 527 B 0 B
assets/js/chunk-web-stories-template-57.js 14.2 kB 0 B
assets/js/chunk-web-stories-template-58-metaData.js 551 B 0 B
assets/js/chunk-web-stories-template-58.js 5.4 kB 0 B
assets/js/chunk-web-stories-template-59-metaData.js 588 B 0 B
assets/js/chunk-web-stories-template-59.js 8.66 kB 0 B
assets/js/chunk-web-stories-template-6-metaData.js 566 B 0 B
assets/js/chunk-web-stories-template-6.js 6.87 kB 0 B
assets/js/chunk-web-stories-template-60-metaData.js 513 B 0 B
assets/js/chunk-web-stories-template-60.js 8.83 kB 0 B
assets/js/chunk-web-stories-template-7-metaData.js 566 B 0 B
assets/js/chunk-web-stories-template-7.js 7.08 kB 0 B
assets/js/chunk-web-stories-template-8-metaData.js 566 B 0 B
assets/js/chunk-web-stories-template-8.js 8.28 kB 0 B
assets/js/chunk-web-stories-template-9-metaData.js 577 B 0 B
assets/js/chunk-web-stories-template-9.js 8.17 kB 0 B
assets/js/chunk-web-stories-templates.js 584 B 0 B
assets/js/chunk-web-stories-textset-0.js 4.57 kB 0 B
assets/js/chunk-web-stories-textset-1.js 5.57 kB 0 B
assets/js/chunk-web-stories-textset-2.js 6.79 kB 0 B
assets/js/chunk-web-stories-textset-3.js 12.6 kB 0 B
assets/js/chunk-web-stories-textset-4.js 3.88 kB 0 B
assets/js/chunk-web-stories-textset-5.js 5.24 kB 0 B
assets/js/chunk-web-stories-textset-6.js 4.96 kB 0 B
assets/js/chunk-web-stories-textset-7.js 8.77 kB 0 B
assets/js/generateBlurhash.worker.worker.js 1.16 kB 0 B
assets/js/web-stories-activation-notice.js 22.7 kB 0 B
assets/js/web-stories-carousel.js 9.87 kB 0 B
assets/js/web-stories-embed.js 20 B 0 B
assets/js/web-stories-lightbox.js 7.31 kB 0 B
assets/js/web-stories-tinymce-button.js 9.78 kB 0 B
assets/js/web-stories-widget.js 554 B 0 B

compressed-size-action

github-actions[bot] avatar Jun 20 '24 07:06 github-actions[bot]

Plugin builds for 682a36fce622b36b908afa05a67022d9ef8de8d4 are ready :bellhop_bell:!

googleforcreators-bot avatar Jun 20 '24 07:06 googleforcreators-bot

Why is the bundle size increasing like that? I was hoping for the opposite or equal. Can you analyze that a bit?

swissspidy avatar Jun 20 '24 10:06 swissspidy

Why is the bundle size increasing like that? I was hoping for the opposite or equal. Can you analyze that a bit?

Running npm run build locally gave:

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets:
  web-stories-editor.js (4.11 MiB)
  5803.js (331 KiB)
  911.js (704 KiB)

Taking a look into the built web-stories-editor.js, I found many SpriteSymbolComponents:

(e, t, a) => {
    "use strict";
    a.d(t, { default: () => CheckboxSpriteSymbolComponent });
    var n = a(96540),
        o = a(12897),
        r = a.n(o),
        l = a(55042),
        C = a.n(l),
        i = a(46468);
    function s() {
        return (
            (s = Object.assign
                ? Object.assign.bind()
                : function (e) {
                      for (var t = 1; t < arguments.length; t++) {
                          var a = arguments[t];
                          for (var n in a)
                              Object.prototype.hasOwnProperty.call(a, n) &&
                                  (e[n] = a[n]);
                      }
                      return e;
                  }),
            s.apply(this, arguments)
        );
    }
    const c = new (r())({
        id: "checkbox",
        use: "checkbox-usage",
        viewBox: "0 0 32 32",
        content:
            '<symbol xmlns="http://www.w3.org/2000/svg" fill="none" viewBox="0 0 32 32" id="checkbox"><path fill="currentColor" fill-rule="evenodd" d="M23.897 18.146a.5.5 0 0 1 .707.708l-4.493 4.493-.007.007a.5.5 0 0 1-.708 0l-.006-.007-2.494-2.493a.5.5 0 0 1 .708-.708l2.146 2.147z" clip-rule="evenodd" /><path stroke="currentColor" stroke-linecap="round" stroke-linejoin="round" d="M20.333 15.5v-5.083C20.333 9.634 19.7 9 18.917 9h-6.5C11.634 9 11 9.634 11 10.417v11.166c0 .783.634 1.417 1.417 1.417H14.5" /></symbol>',
    });
};

I think our svgs are ending up in JS :(

Swanand01 avatar Jun 20 '24 10:06 Swanand01

Can this be fixed? The idea behind this whole ticket was to improve performance by reducing bundle size. An icon should just reference the definition in the sprite and not include the svg itself as well.

Maybe need to consult the svg-sprite-loader repo examples again.

swissspidy avatar Jun 20 '24 11:06 swissspidy

Okay this is what I have found so far:

  1. Using extract: true in the svg-sprite-loader options generates the SVG sprite file with all icons, which wasn't happening earlier.
  2. Not using extract: true puts the symbols in the generated JS file.
  3. This example uses a custom runtime generator with extract mode, which is what we want, but it does not return a component from the runtime generator.
  4. The build step should generate a sprite.svg file in the assets/js/ folder, but I don't see that happening here. It appears when I run npm run build locally.
  5. The SVGs ending up in the generated JS files is probably due to the component in runtime generator.

@swissspidy Your inputs on this would be of great help 😄

Swanand01 avatar Jun 20 '24 13:06 Swanand01

So we need extract an a custom runtime returning a component, is that correct? Are there any examples for that somewhere or other repositiries using that?

The build step should generate a sprite.svg file in the assets/js/ folder, but I don't see that happening https://github.com/GoogleForCreators/web-stories-wp/pull/13730#issuecomment-2180021912. It appears when I run npm run build locally.

These bubdle size PR comments are currently configured to only list JS and CSS files. That‘s why you don‘t see SVGs there. This can be changed in the workflow file.

swissspidy avatar Jun 21 '24 07:06 swissspidy

So we need extract an a custom runtime returning a component, is that correct? Are there any examples for that somewhere or other repositiries using that?

Yes, exactly. But it seems this is putting the symbol components inside the js bundle, which is increasing its size. The last option would be just to generate the SVG sprite file, and use a custom Icon component that references it, instead of importing svg files.

Swanand01 avatar Jun 21 '24 07:06 Swanand01

Maybe you can find some existing repos/examples so we can learn how they are doing it. If not, let's put this ticket aside for now as it seems bigger as anticipated.

These bubdle size PR comments are currently configured to only list JS and CSS files. That‘s why you don‘t see SVGs there. This can be changed in the workflow file.

Here's where that would need to be changed:

https://github.com/GoogleForCreators/web-stories-wp/blob/0be152e0443d0ba6b6df20a2121e89af3a18adae/.github/workflows/build-and-deploy.yml#L112-L120

swissspidy avatar Jun 21 '24 08:06 swissspidy