[web] Start using icons from Material Symbols #383
Conversation
To start using the new Material Symbols icons https://fonts.google.com/icons
It transforms SVG icons into React components, which will initially help using Material Symbol icons imported from @material-symbols/svg package
And stop using icons from eos-icons-react and @patternfly. The only exception, by now, is the "LoadingIcon", which does not come from @material-icon but from a local .svg file borrowed from https://github.com/n3r4zzurr0/svg-spinners
For reducing imports and easing the way to use icon, using the original icon name for importing it.
Because at this time TypeScript's React types rejects function components that does not return null | JSX.Element. See microsoft/TypeScript#51328 RFC and follow related links to known more. Other links: * DefinitelyTyped/DefinitelyTyped#18051 * DefinitelyTyped/DefinitelyTyped#62876 * https://stackoverflow.com/a/70895599
To avoid breaking unit test because of the string mock.
This helps to simplify the Jest configuration and to reduce the mocking too. It's somehow related to the fact that Jest does not fully support resource queries, see jestjs/jest#6282
dgdavid
changed the title
dgdavid
changed the title
Unfortunately, I was working under the wrong assumption when checking the generated bundle: it was the development bundle, not the production one. Rather than generating it with
quotation from the Conclusions chapter of Webpack's tree-shaking page With Click to show/hide metrics of production build for both branches
However, I would like to merge these changes since they are significant improvements to the code base, especially in development mode 😜 |
dgdavid
changed the title
imobachgs
left a comment
imobachgs
left a comment
There was a problem hiding this comment.
LGTM. It is a big improvement, and we can switch to a different set of icons if needed in the future.
|
Oh, I forgot: please, add an entry to the changes file. |
3 participants
Problem
We've noticed that the bundle dist/index.js file is rather big, a bit over 21M
Upon closer inspection, it appears that the EOS Icons package is being imported in full, rather than just the used icons.
I.e., for some reason the expected tree-shaking is not working (as expected).See #383 (comment)That, for sure, has an impact in the needed time to have the UI ready for the user the first time. In fact, the First Contentful Paint (FCP) and Largest Contentful Paint (LCP) web vitals metrics are not good.See #383 (comment)Solution
Despite not being a size or performance issue with the bundle generated in the production mode see comment below, it seems like a good time to start using the recently released Material Symbols icon set, but importing only the ones needed from https://github.com/marella/material-symbols instead of directly using the variable font.
To ease future updates, the import of icons has also been centralized into the layout/
Iconcomponent which (hopefully) will help to avoid having to change all components with icons.Why not using the Material Symbols variable font directly?
Because at the moment we're using exactly 23 icons from that font (the animated Loading icon is a custom one). Thus, having all of them on hand "just in case" does not make sense even though the current approach loses most of the main advantages of variable fonts.
Testing
Changes has been tested manually.
To avoid breaking the unit tests, some Jest configuration changes and mocking were needed to deal with the @svgr/webpack package used to transform SVG files into React components.
Screenshots
Next steps
Web vitals metrics gathered by https://github.com/GoogleChrome/web-vitals
Treemaps generated by https://github.com/webpack-contrib/webpack-bundle-analyzer
Sizes were collected with https://github.com/robertknight/webpack-bundle-size-analyzer