-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
@zooniverse/react-components: improvements to the output bundle size #3761
Conversation
a827a95
to
45cfc9d
Compare
Grommet Icons has a bell icon ( |
45cfc9d
to
8fe1816
Compare
I assume those are the only two icons that use Font Awesome in lib-react-components. Could we replace them in this PR with Grommet icons to completely remove Font Awesome? |
According to #275 this is for parity with the old header, but it seems like a lot of code for two icons. |
Ah I see that makes sense. Agreed that it's not ideal to keep those icons around though. |
8fe1816
to
ea83291
Compare
It's tempting to simply copy the SVG paths for those two icons into the code for the header menu. I'm pushing up another change, which loads in The classifier build is also 1.5MB, which leads to very heavy page weights in the project app. |
ea83291
to
788fec4
Compare
Tests that expect labels like 'Add to favourites' are failing now because the default labels in the output library will be the translation keys. Easy enough to fix. |
75cd247
to
bdea436
Compare
Localised text still works when I build and run the project app locally. Tested with PH-TESS in English and Spanish. |
9a68fb9
to
4fa5bf6
Compare
Replacing |
b4cbcf2
to
e9fbd4f
Compare
I've switched the Most of that 500k is probably Markdownz, so we could consider splitting that out of the main library for consumers that don't use Zooniverse markdown. |
e9fbd4f
to
c210484
Compare
9df5c27
to
0946c55
Compare
0946c55
to
eeb5f2a
Compare
...b-react-components/src/ZooHeader/components/SignedInUserNavigation/SignedInUserNavigation.js
Outdated
Show resolved
Hide resolved
c58f2ba
to
72ceac6
Compare
97e4f24
to
f0aa4a9
Compare
I think the problem with documenting peer deps in the Readme is versioning. Mapping Viz uses either version 1.0 or 1.1. The version here is 1.2 so changes here won’t be relevant to Mapping Viz at the moment. Looking at other libraries we use, like Grommet, peer dependencies aren’t explicitly spelled out in their Readmes either. I’m still not sure whether the |
5f49469
to
4c7a7c1
Compare
- add webpack bundle analyser - import only the bell and envelope icons from Font Awesome. - replace `@fortawesome/react-fontawesome` and `@fortawesome/fontawesome-svg-core` with Grommet Icons `Blank`. - import only the lodash utilities that are used in the code. - mark `i18next` and `react-18next` as external dependencies in libraries. - update tests to query elements by their translation keys. - replace `mime-types` with `mime/lite`. Should reduce the build size by ~350k.
4c7a7c1
to
4053d9f
Compare
I'm going to close this in favour of #3900, which is producing smaller, faster builds. |
i18next
andreact-18next
as external dependencies in libraries.mime-types
withmime/lite
.@fortawesome/react-fontawesome
and@fortawesome/fontawesome-svg-core
with Grommet IconsBlank
.Should reduce the build size by ~350k.
Package
lib-classifier
lib-react-components
Linked Issue and/or Talk Post
How to Review
Build the library with
yarn build
and look at the bundle size (dist/main.js
). The built library should still work when imported into the classifier and NextJS apps.Checklist
PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.
General
yarn panic && yarn bootstrap
ordocker-compose up --build
and FEM works as expectedGeneral UX
Example Staging Project: i-fancy-cats