openmrs-esm-core
openmrs-esm-core copied to clipboard
openmrs
(feat) Add CarbonMRS icon set and React components
Requirements
- [x] This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as
feat,fix, orchore, among others). See existing PR titles for inspiration.
For changes to apps
- [x] My work conforms to the O3 Styleguide and design documentation.
If applicable
- [ ] My work includes tests or is validated by existing tests.
- [ ] I have updated the esm-framework mock to reflect any API changes I have made.
Summary
This is a draft PR for implementing the CarbonMRS icon set and replacing most uses of the Carbon icons with the custom icons. The root idea here is to leverage the existing SVG sprites system, so that all svgs are loaded only once and simply cloned in place as needed.
Currently, the main limitation of this implementation is the lack of actual SVG icons (the files here are simply placeholders), so things I'm looking for feedback on are:
- Is it too much to load 50-ish SVGs up front? (Especially curious about the size implications this has for the
openmrs.jsfile). - Does the naming convention for icons here make sense? (These icon names are mostly directly taken from CarbonMRS / the Zero Height docs with a couple of aliases where I thought it made sense)
- Are there other properties we should support on the simple
iconcomponent?
Screenshots
Related Issue
Other
Size Change: -624 kB (-15.29%) 👏
Total Size: 3.46 MB
| Filename | Size | Change | |
|---|---|---|---|
packages/apps/esm-devtools-app/dist/344.js |
0 B | -27.3 kB (removed) | 🏆 |
packages/apps/esm-implementer-tools-app/dist/15.js |
0 B | -62.2 kB (removed) | 🏆 |
packages/apps/esm-login-app/dist/836.js |
0 B | -22.5 kB (removed) | 🏆 |
packages/apps/esm-offline-tools-app/dist/59.js |
0 B | -56.5 kB (removed) | 🏆 |
packages/apps/esm-primary-navigation-app/dist/553.js |
0 B | -23.3 kB (removed) | 🏆 |
packages/shell/esm-app-shell/dist/openmrs.a62f03dda9099896.js |
0 B | -383 kB (removed) | 🏆 |
ℹ️ View Unchanged
| Filename | Size | Change | |
|---|---|---|---|
packages/apps/esm-devtools-app/dist/630.js |
2.69 kB | +2 B (+0.07%) | |
packages/apps/esm-devtools-app/dist/667.js |
6.96 kB | 0 B | |
packages/apps/esm-devtools-app/dist/735.js |
2.63 kB | 0 B | |
packages/apps/esm-devtools-app/dist/788.js |
42.9 kB | 0 B | |
packages/apps/esm-devtools-app/dist/875.js |
9.84 kB | -19 B (-0.19%) | |
packages/apps/esm-devtools-app/dist/884.js |
15.2 kB | 0 B | |
packages/apps/esm-devtools-app/dist/885.js |
27.1 kB | 0 B | |
packages/apps/esm-devtools-app/dist/889.js |
159 kB | -5.46 kB (-3.31%) | |
packages/apps/esm-devtools-app/dist/988.js |
328 B | 0 B | |
packages/apps/esm-devtools-app/dist/main.js |
3.14 kB | -2 B (-0.06%) | |
packages/apps/esm-devtools-app/dist/openmrs-esm-devtools-app.js |
3.19 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/271.js |
716 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/319.js |
637 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/426.js |
24.8 kB | +2 B (+0.01%) | |
packages/apps/esm-implementer-tools-app/dist/460.js |
735 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/482.js |
15.2 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/523.js |
5.73 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/528.js |
133 kB | -45 B (-0.03%) | |
packages/apps/esm-implementer-tools-app/dist/56.js |
3.07 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/560.js |
13.8 kB | -21 B (-0.15%) | |
packages/apps/esm-implementer-tools-app/dist/574.js |
560 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/587.js |
2.92 kB | -5 B (-0.17%) | |
packages/apps/esm-implementer-tools-app/dist/620.js |
126 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/625.js |
562 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/644.js |
717 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/657.js |
7.01 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/71.js |
6.97 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/735.js |
2.63 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/757.js |
560 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/788.js |
42.9 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/791.js |
284 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/807.js |
559 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/833.js |
681 B | 0 B | |
packages/apps/esm-implementer-tools-app/dist/86.js |
0 B | -6.71 kB (removed) | 🏆 |
packages/apps/esm-implementer-tools-app/dist/889.js |
159 kB | -5.46 kB (-3.31%) | |
packages/apps/esm-implementer-tools-app/dist/891.js |
61.3 kB | 0 B | |
packages/apps/esm-implementer-tools-app/dist/main.js |
78.1 kB | -931 B (-1.18%) | |
packages/apps/esm-implementer-tools-app/dist/openmrs-esm-implementer-tools-app.js |
3.31 kB | +1 B (+0.03%) | |
packages/apps/esm-login-app/dist/111.js |
1.22 kB | 0 B | |
packages/apps/esm-login-app/dist/126.js |
2.5 kB | 0 B | |
packages/apps/esm-login-app/dist/173.js |
1.22 kB | 0 B | |
packages/apps/esm-login-app/dist/224.js |
256 B | 0 B | |
packages/apps/esm-login-app/dist/236.js |
272 B | 0 B | |
packages/apps/esm-login-app/dist/240.js |
364 B | 0 B | |
packages/apps/esm-login-app/dist/271.js |
718 B | 0 B | |
packages/apps/esm-login-app/dist/272.js |
264 B | 0 B | |
packages/apps/esm-login-app/dist/319.js |
679 B | 0 B | |
packages/apps/esm-login-app/dist/336.js |
234 B | 0 B | |
packages/apps/esm-login-app/dist/363.js |
30.5 kB | -11 B (-0.04%) | |
packages/apps/esm-login-app/dist/460.js |
737 B | 0 B | |
packages/apps/esm-login-app/dist/539.js |
298 B | 0 B | |
packages/apps/esm-login-app/dist/56.js |
3.06 kB | 0 B | |
packages/apps/esm-login-app/dist/574.js |
577 B | 0 B | |
packages/apps/esm-login-app/dist/625.js |
579 B | 0 B | |
packages/apps/esm-login-app/dist/627.js |
257 B | 0 B | |
packages/apps/esm-login-app/dist/63.js |
16.5 kB | 0 B | |
packages/apps/esm-login-app/dist/644.js |
718 B | 0 B | |
packages/apps/esm-login-app/dist/667.js |
6.96 kB | 0 B | |
packages/apps/esm-login-app/dist/673.js |
284 B | 0 B | |
packages/apps/esm-login-app/dist/735.js |
2.63 kB | 0 B | |
packages/apps/esm-login-app/dist/757.js |
660 B | 0 B | |
packages/apps/esm-login-app/dist/788.js |
42.9 kB | 0 B | |
packages/apps/esm-login-app/dist/807.js |
897 B | 0 B | |
packages/apps/esm-login-app/dist/833.js |
684 B | 0 B | |
packages/apps/esm-login-app/dist/884.js |
15.2 kB | 0 B | |
packages/apps/esm-login-app/dist/889.js |
159 kB | -5.46 kB (-3.31%) | |
packages/apps/esm-login-app/dist/905.js |
22 kB | 0 B | |
packages/apps/esm-login-app/dist/main.js |
56.5 kB | -515 B (-0.9%) | |
packages/apps/esm-login-app/dist/openmrs-esm-login-app.js |
3.37 kB | +2 B (+0.06%) | |
packages/apps/esm-offline-tools-app/dist/271.js |
1.18 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/319.js |
1.13 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/460.js |
1.29 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/56.js |
3.07 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/574.js |
1.04 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/625.js |
1.04 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/63.js |
16.5 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/644.js |
1.18 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/667.js |
6.96 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/735.js |
2.63 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/757.js |
1.2 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/788.js |
42.9 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/807.js |
1.11 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/833.js |
1.21 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/884.js |
15.2 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/889.js |
159 kB | -5.46 kB (-3.31%) | |
packages/apps/esm-offline-tools-app/dist/922.js |
90.9 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/975.js |
56.6 kB | 0 B | |
packages/apps/esm-offline-tools-app/dist/main.js |
147 kB | +83 B (+0.06%) | |
packages/apps/esm-offline-tools-app/dist/openmrs-esm-offline-tools-app.js |
3.29 kB | +1 B (+0.03%) | |
packages/apps/esm-primary-navigation-app/dist/271.js |
267 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/319.js |
237 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/380.js |
22.8 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/460.js |
264 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/574.js |
230 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/625.js |
232 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/63.js |
16.5 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/644.js |
267 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/667.js |
6.97 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/735.js |
2.64 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/757.js |
238 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/762.js |
7.61 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/788.js |
42.9 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/807.js |
290 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/833.js |
257 B | 0 B | |
packages/apps/esm-primary-navigation-app/dist/884.js |
15.2 kB | 0 B | |
packages/apps/esm-primary-navigation-app/dist/889.js |
159 kB | -5.46 kB (-3.31%) | |
packages/apps/esm-primary-navigation-app/dist/958.js |
22.7 kB | -9 B (-0.04%) | |
packages/apps/esm-primary-navigation-app/dist/main.js |
47 kB | -533 B (-1.12%) | |
packages/apps/esm-primary-navigation-app/dist/openmrs-esm-primary-navigation-app.js |
3.23 kB | 0 B | |
packages/framework/esm-api/dist/openmrs-esm-api.js |
16.3 kB | 0 B | |
packages/framework/esm-config/dist/openmrs-esm-module-config.js |
8.02 kB | 0 B | |
packages/framework/esm-context/dist/openmrs-esm-context.js |
1.1 kB | 0 B | |
packages/framework/esm-dynamic-loading/dist/openmrs-esm-dynamic-loading.js |
2.75 kB | 0 B | |
packages/framework/esm-error-handling/dist/openmrs-esm-error-handling.js |
889 B | 0 B | |
packages/framework/esm-extensions/dist/openmrs-esm-extensions.js |
8.14 kB | 0 B | |
packages/framework/esm-feature-flags/dist/openmrs-esm-feature-flags.js |
1.67 kB | 0 B | |
packages/framework/esm-framework/dist/126.openmrs-esm-framework.js |
2.47 kB | 0 B | |
packages/framework/esm-framework/dist/278.openmrs-esm-framework.js |
14.5 kB | 0 B | |
packages/framework/esm-framework/dist/530.openmrs-esm-framework.js |
2.92 kB | 0 B | |
packages/framework/esm-framework/dist/619.openmrs-esm-framework.js |
6.49 kB | 0 B | |
packages/framework/esm-framework/dist/645.openmrs-esm-framework.js |
9.31 kB | 0 B | |
packages/framework/esm-framework/dist/680.openmrs-esm-framework.js |
6.13 kB | 0 B | |
packages/framework/esm-framework/dist/735.openmrs-esm-framework.js |
2.66 kB | 0 B | |
packages/framework/esm-framework/dist/788.openmrs-esm-framework.js |
42.9 kB | 0 B | |
packages/framework/esm-framework/dist/openmrs-esm-framework.js |
486 kB | +1.25 kB (+0.26%) | |
packages/framework/esm-globals/dist/openmrs-esm-globals.js |
796 B | 0 B | |
packages/framework/esm-navigation/dist/openmrs-esm-navigation.js |
9.35 kB | 0 B | |
packages/framework/esm-offline/dist/openmrs-esm-offline.js |
34.4 kB | 0 B | |
packages/framework/esm-react-utils/dist/openmrs-esm-react-utils.js |
15.6 kB | 0 B | |
packages/framework/esm-routes/dist/openmrs-esm-utils.js |
1.46 kB | 0 B | |
packages/framework/esm-state/dist/openmrs-esm-state.js |
921 B | 0 B | |
packages/framework/esm-styleguide/dist/openmrs-esm-styleguide.js |
48.8 kB | -5.02 kB (-9.32%) | ✅ |
packages/framework/esm-translations/dist/openmrs-esm-core-translations.js |
1.59 kB | 0 B | |
packages/framework/esm-utils/dist/openmrs-esm-utils.js |
18.2 kB | 0 B | |
packages/shell/esm-app-shell/dist/11c63b65f96a8718.js |
499 B | 0 B | |
packages/shell/esm-app-shell/dist/1e0131662341578e.js |
645 B | 0 B | |
packages/shell/esm-app-shell/dist/2916d0aa7a9d5dc8.js |
544 B | 0 B | |
packages/shell/esm-app-shell/dist/4a3e954c45d63305.js |
645 B | 0 B | |
packages/shell/esm-app-shell/dist/56c2295bc732ae32.js |
722 B | 0 B | |
packages/shell/esm-app-shell/dist/651172ae1548469c.js |
499 B | 0 B | |
packages/shell/esm-app-shell/dist/7ba811602c7a0036.js |
0 B | -1.58 kB (removed) | 🏆 |
packages/shell/esm-app-shell/dist/98343ad4bb547c48.js |
499 B | 0 B | |
packages/shell/esm-app-shell/dist/af598ac97d115dba.js |
1.58 kB | 0 B | |
packages/shell/esm-app-shell/dist/b5151d35f680b40a.js |
3.82 kB | 0 B | |
packages/shell/esm-app-shell/dist/ba933133ad512cac.js |
499 B | 0 B | |
packages/shell/esm-app-shell/dist/dc5587cbc5f783ec.js |
6.91 kB | 0 B | |
packages/shell/esm-app-shell/dist/ecec35bb68a5d6b9.js |
0 B | -6.91 kB (removed) | 🏆 |
packages/shell/esm-app-shell/dist/fa89289c9d9645c9.js |
519 B | 0 B | |
packages/shell/esm-app-shell/dist/openmrs.e766ce6aefda5d2a.js |
382 kB | 0 B | |
packages/shell/esm-app-shell/dist/service-worker.js |
45.9 kB | -4 B (-0.01%) | |
packages/tooling/openmrs/dist/cli.js |
2.88 kB | 0 B | |
packages/tooling/openmrs/dist/commands/assemble.js |
3.21 kB | 0 B | |
packages/tooling/openmrs/dist/commands/build.js |
1.34 kB | 0 B | |
packages/tooling/openmrs/dist/commands/debug.js |
545 B | 0 B | |
packages/tooling/openmrs/dist/commands/develop.js |
2.59 kB | 0 B | |
packages/tooling/openmrs/dist/commands/index.js |
438 B | 0 B | |
packages/tooling/openmrs/dist/commands/start.js |
851 B | 0 B | |
packages/tooling/openmrs/dist/index.js |
517 B | 0 B | |
packages/tooling/openmrs/dist/runner.js |
637 B | 0 B | |
packages/tooling/openmrs/dist/utils/config.js |
728 B | 0 B | |
packages/tooling/openmrs/dist/utils/debugger.js |
576 B | 0 B | |
packages/tooling/openmrs/dist/utils/dependencies.js |
648 B | 0 B | |
packages/tooling/openmrs/dist/utils/helpers.js |
395 B | 0 B | |
packages/tooling/openmrs/dist/utils/importmap.js |
3.07 kB | 0 B | |
packages/tooling/openmrs/dist/utils/index.js |
444 B | 0 B | |
packages/tooling/openmrs/dist/utils/logger.js |
368 B | 0 B | |
packages/tooling/openmrs/dist/utils/npmConfig.js |
830 B | 0 B | |
packages/tooling/openmrs/dist/utils/untar.js |
722 B | 0 B | |
packages/tooling/openmrs/dist/utils/variables.js |
192 B | 0 B | |
packages/tooling/openmrs/dist/utils/webpack.js |
278 B | 0 B | |
packages/tooling/webpack-config/dist/index.js |
3.61 kB | 0 B |
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Is it too much to load 50-ish SVGs up front? (Especially curious about the size implications this has for the openmrs.js file).
Seems like it would be great if we could avoid it, right? Even if all the icons shared their own bundle that would seem to be better than having them in openmrs.js.
Does the naming convention for icons here make sense? (These icon names are mostly directly taken from CarbonMRS / the Zero Height docs with a couple of aliases where I thought it made sense)
Yes, I think they do.
Alright, my impression is the e2e test failures here are unavoidable because of how the Docker image used here is constructed. That's because this PR introduces several new components into the styleguide, which are now used by all apps, but the Dockerfile is built using the published version of the app shell, and so those components are not present. That should be fixed, but I'm going to mark this as ready for review.
Is the bundle size change estimate accurate, @ibacher, in the sense that this change reduces the bundle size rather than inflating it?
@denniskigen Huh! I anticipated the shrinking in the apps themselves, since we no longer depend on @carbon/react/icons and because @carbon/react/icons is packaged in such a way that it doesn't really get tree-shaken properly. What I didn't expect was this:
However, the icons I added don't consume much more space than they did before, despite having almost twice as many:
With PR
Main
The React components are much more heavy-weight:
But it seems that adding the svgo-loader, which optimizes SVGs, made a huge amount of savings in the size of the logo:
With PR
Main
So... yeah, this looks legit to me...
~~Should it be straightforward to test locally? Getting these placeholders instead of the actual icons:~~
Well, ignore all of that. I hadn't read the PR description 🤦🏼 At least I've verified that it works as expected 😅
Should it be straightforward to test locally?
You're seeing exactly what you should be seeing. All the icon files are current just that placeholder image. I reverted things back to where it was initially, which was before we had any of the actual svgs in the design docs.
I made some slight adjustments to this to try to reduce the chance of mistakenly importing the various SVGs, e.g., in Jest tests. This entailed some minor updates to the description of adding an icon or pictogram, which I've made above.