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

Remove iconochive #9262

Closed
cdrini opened this issue May 13, 2024 · 5 comments · Fixed by #9310
Closed

Remove iconochive #9262

cdrini opened this issue May 13, 2024 · 5 comments · Fixed by #9310
Assignees
Labels
Good First Issue Easy issue. Good for newcomers. [managed] Lead: @RayBB Issues overseen by Ray (Onboarding & Documentation Lead) [manages] Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. Priority: 3 Issues that we can consider at our leisure. [managed] Team: Front-end Issues belonging to the Front-end team [experimental tag] Theme: Performance Issues related to UI or Server performance. [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]

Comments

@cdrini
Copy link
Collaborator

cdrini commented May 13, 2024

We include a decent amount of iconochive CSS which we don't use ; we don't use iconochive anywhere. Let's delete it.

Files to delete:

  • static/css/components/iconochive.less
  • static/fonts/Iconochive-*

References to delete (delete just where iconochive is mentioned not the whole file):

  • static/css/legacy.less
  • static/css/less/font-families.less

Evaluation:

  • Before doing the deletions, do a make less and then run du -sh static/build/page-*.css. Then do the deletions, run the two commands again, and compare the sizes!
  • Also update bundlesize.config.json for the css files to be the new size +1 kb.
@cdrini cdrini added Good First Issue Easy issue. Good for newcomers. [managed] Theme: Performance Issues related to UI or Server performance. [managed] Priority: 2 Important, as time permits. [managed] Team: Front-end Issues belonging to the Front-end team [experimental tag] Lead: @RayBB Issues overseen by Ray (Onboarding & Documentation Lead) [manages] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed] labels May 13, 2024
@danwoods
Copy link
Contributor

I'd like to grab this ✋

@danwoods
Copy link
Contributor

Playing with getting the repo setup and encountering the following issues after getting things running in Docker and building the assets (via docker compose run --rm home npm run build-assets):

Running make less from the top level directory returns make: *** No rule to make target less'. Stop.Runningdu -sh static/build/page-.cssfrom the top level directory returnsdu: static/build/page-.css: No such file or directory`

@scottbarnes
Copy link
Collaborator

Hi, @danwoods:

I think Drini may have meant to run make css first, though if you run that outside of the container it may rely on something you don't have and may not want to install (parallel).

If you run make less inside the container it will work, but you'll need to have the container running and use docker compose exec for this, as doing docker compose run will I think not persist, though I may be wrong. E.g.:

❯ docker compose exec web bash
WARN[0000] The "HOST" variable is not set. Defaulting to a blank string. 
openlibrary@48c15bb0e470:/openlibrary$ make css
mkdir -p static/build
parallel --verbose -q npx lessc {} static/build/{/.}.css --clean-css="--s1 --advanced" ::: static/css/page-admin.less static/css/page-barcodescanner.less static/css/page-book-widget.less static/css/page-book.less static/css/page-design.less static/css/page-dev.less static/css/page-edit.less static/css/page-form.less static/css/page-home.less static/css/page-list-edit.less static/css/page-lists.less static/css/page-plain.less static/css/page-subject.less static/css/page-team.less static/css/page-user.less
npx lessc static/css/page-admin.less static/build/page-admin.css '--clean-css=--s1 --advanced'
npx lessc static/css/page-barcodescanner.less static/build/page-barcodescanner.css '--clean-css=--s1 --advanced'
[...]

You probably also need to run the du command inside the container too. That said, you won't be able to delete from within the container, so do that outside.

So the capsule summary is run make css and du inside the container, and delete and edit files outside. Deletions and edits outside should be should be immediately reflected inside. Also, probably use git rm to make things easier.

Please follow up with any questions, and hopefully I have not led you further astray. :)

@mekarpeles mekarpeles added Priority: 3 Issues that we can consider at our leisure. [managed] and removed Priority: 2 Important, as time permits. [managed] labels May 16, 2024
@danwoods
Copy link
Contributor

Thanks @scottbarnes! Will check this out 👍

@github-actions github-actions bot added the Needs: Response Issues which require feedback from lead label May 17, 2024
@scottbarnes
Copy link
Collaborator

I don't know why I said using docker compose run probably wouldn't work, because I think it probably would. Either way one or both will. :)

@mekarpeles mekarpeles removed the Needs: Response Issues which require feedback from lead label May 20, 2024
@RayBB RayBB added the Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. label May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Easy issue. Good for newcomers. [managed] Lead: @RayBB Issues overseen by Ray (Onboarding & Documentation Lead) [manages] Needs: Staff / Internal Reviewed a PR but don't have merge powers? Use this. Priority: 3 Issues that we can consider at our leisure. [managed] Team: Front-end Issues belonging to the Front-end team [experimental tag] Theme: Performance Issues related to UI or Server performance. [managed] Type: Refactor/Clean-up Issues related to reorganization/clean-up of data or code (e.g. for maintainability). [managed]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants