Skip to content
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

Removing iconochive #9310

Merged

Conversation

danwoods
Copy link
Contributor

Closes #9262

refactor

Technical

The new file sizes are larger than the current maxsizes in bundlesize.config.json.

Testing

Searching for iconochive should return no results.

Screenshot

Screenshot 2024-05-21 at 11 27 09 AM Screenshot 2024-05-21 at 11 26 30 AM

Stakeholders

@cdrini
@scottbarnes


Additional Notes:
Stats before deletion:

128K	static/build/page-admin.css
40K	  static/build/page-barcodescanner.css
4.0K	static/build/page-book-widget.css
64K	  static/build/page-book.css
124K	static/build/page-design.css
4.0K	static/build/page-dev.css
124K	static/build/page-edit.css
124K	static/build/page-form.css
36K	  static/build/page-home.css
124K	static/build/page-list-edit.css
136K	static/build/page-lists.css
128K	static/build/page-plain.css
44K	  static/build/page-subject.css
28K	  static/build/page-team.css
136K	static/build/page-user.css

Deleted mentioned files and removed references in mentioned files. Now nothing comes up when searching the codebase for “iconochive”.

Re-ran make css and du -sh static/build/page-*.css

Stats after deletion:

124K	static/build/page-admin.css
40K	  static/build/page-barcodescanner.css
4.0K	static/build/page-book-widget.css
64K	  static/build/page-book.css
120K	static/build/page-design.css
4.0K	static/build/page-dev.css
120K	static/build/page-edit.css
120K	static/build/page-form.css
36K	  static/build/page-home.css
120K	static/build/page-list-edit.css
132K	static/build/page-lists.css
120K	static/build/page-plain.css
44K	  static/build/page-subject.css
28K	  static/build/page-team.css
128K	static/build/page-user.css

Updated bundlesize.config.json references. NOTE Many of the existing maxSizes were much smaller than the new sizes.

Clicked around. Not super familiar with the site, but nothing looked esp broken.

Ran tests. All passed.

@RayBB
Copy link
Collaborator

RayBB commented May 21, 2024

@danwoods could you make a table of before, after, and difference the CSS files? I think you can do this by asking chatgpt :) if it's too much work don't worry

@danwoods
Copy link
Contributor Author

CSS File Name Before (KB) After (KB) Difference (KB)
static/build/page-admin.css 128 124 -4
static/build/page-barcodescanner.css 40 40 0
static/build/page-book-widget.css 4 4 0
static/build/page-book.css 64 64 0
static/build/page-design.css 124 120 -4
static/build/page-dev.css 4 4 0
static/build/page-edit.css 124 120 -4
static/build/page-form.css 124 120 -4
static/build/page-home.css 36 36 0
static/build/page-list-edit.css 124 120 -4
static/build/page-lists.css 136 132 -4
static/build/page-plain.css 128 120 -8
static/build/page-subject.css 44 44 0
static/build/page-team.css 28 28 0
static/build/page-user.css 136 128 -8

@RayBB
Copy link
Collaborator

RayBB commented May 22, 2024

@danwoods thanks. I realize @cdrini gave some wrong instructions.
Instead of running that du -sh static/build/page-*.css command we want to run npx bundlesize

I ran did this and here are the results - a bit more in line with what we'd expect:

Filename Before Size After Size Difference
static/build/page-admin.css 25.81KB 24.59KB -1.22KB
static/build/page-book.css 13.32KB 13.32KB 0KB
static/build/page-edit.css 25.38KB 24.15KB -1.23KB
static/build/page-form.css 25.33KB 24.09KB -1.24KB
static/build/page-home.css 7.58KB 7.58KB 0KB
static/build/page-plain.css 25.48KB 24.26KB -1.22KB
static/build/page-subject.css 8.84KB 8.84KB 0KB
static/build/page-user.css 27.36KB 26.09KB -1.27KB

So, in this case, you can update the bundlesizes for just these few files to be a tiny bit smaller.

Thanks!

@danwoods
Copy link
Contributor Author

@RayBB Updated bundlesize.config.json with new sizes. That caused the tests (docker compose run --rm home make test) to fail with errors like

─ static/build/page-admin.css
✔ static/build/page-admin.css 24.59KB = 24.59KB gzip

─ static/build/page-book.css
✖ static/build/page-book.css 13.32KB > 13.32KB gzip

─ static/build/page-edit.css
✔ static/build/page-edit.css 24.15KB < 24.15KB gzip

─ static/build/page-form.css
✖ static/build/page-form.css 24.09KB > 24.09KB gzip

─ static/build/page-home.css
✖ static/build/page-home.css 7.58KB > 7.58KB gzip

─ static/build/page-plain.css
✔ static/build/page-plain.css 24.26KB < 24.26KB gzip

─ static/build/page-subject.css
✔ static/build/page-subject.css 8.84KB < 8.84KB gzip

─ static/build/page-user.css
✔ static/build/page-user.css 26.09KB < 26.09KB gzip

23 checks passed, 3 checks failed

ie: Saying 13.32KB > 13.32KB

My assumption is there's some sub KB difference. I rounded the file sizes up to the next KB (the original issue said to add 1KB) and that allowed all tests to pass. Let me know if that's not the correct approach/thinking or if there's anything else!

Copy link
Collaborator

@RayBB RayBB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like the right approach to me now we just need staff to merge!

@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label May 22, 2024
@cdrini
Copy link
Collaborator

cdrini commented May 23, 2024

Awesome nice work folks! 🥳

@cdrini cdrini merged commit dc05746 into internetarchive:master May 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove iconochive
3 participants