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

Minify SVG icons #4181

Merged
merged 7 commits into from
May 28, 2024
Merged

Minify SVG icons #4181

merged 7 commits into from
May 28, 2024

Conversation

coliff
Copy link
Contributor

@coliff coliff commented May 27, 2024

Reduces file size = faster loading

Uses: https://svgo.dev/ via command line

npx svgo --folder=\"src/css/svg/\"

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

Command line output:

image

Reduces file size = faster loading

Uses: https://svgo.dev/ via command line

`npx svgo --folder=\"src/css/svg/\"`
@codecov-commenter
Copy link

codecov-commenter commented May 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.66%. Comparing base (f2135c3) to head (9524112).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4181      +/-   ##
==========================================
+ Coverage   87.62%   87.66%   +0.03%     
==========================================
  Files         242      242              
  Lines       33076    33076              
  Branches     2177     2159      -18     
==========================================
+ Hits        28984    28997      +13     
+ Misses       3109     3108       -1     
+ Partials      983      971      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Collaborator

HarelM commented May 28, 2024

These svgs are copied into the css.
I prefer to keep them readable here, but shrink them as part of the build process instead of at the source.
It's visible that they are not shrinked in the css here:
https://unpkg.com/[email protected]/dist/maplibre-gl.css

So I think it will be better to go that route, what do you think?

This reverts commit 6b45b24.
@coliff
Copy link
Contributor Author

coliff commented May 28, 2024

ah ok, I'm fine if you want to close this then but I think there are a few micro-optimisations with the changes in this PR though.

I set the script to pretty-print them so they are more human-readable... let me know what you think.

Reduces file size = faster loading

Uses: https://svgo.dev/ via command line

`npx svgo --folder=\"src/css/svg/\"`

Revert "Minify SVG icons"

This reverts commit 6b45b24.

SVGO pretty printed

Revert "SVGO pretty printed"

This reverts commit 75a77c0.

don't remove viewBox property
@HarelM
Copy link
Collaborator

HarelM commented May 28, 2024

The only change is the addition of the viewbox, is this a must?

@coliff
Copy link
Contributor Author

coliff commented May 28, 2024

The only change is the addition of the viewbox, is this a must?

I made a single commit to revert a change in removing viewBox, but overall the PR does a lot more... view source diff in GitHub Web UI to see - it makes the SVGs easier to read and visually parse:

image

@HarelM
Copy link
Collaborator

HarelM commented May 28, 2024

I must have looked at something different, not sure how.
LGTM. thanks!

@HarelM HarelM merged commit e1520a6 into maplibre:main May 28, 2024
15 checks passed
@coliff coliff deleted the dev/coliff/minify-svgs branch May 28, 2024 07:35
coliff added a commit to coliff/maplibre-gl-js that referenced this pull request May 29, 2024
I introduced a bug with maplibre#4181 by removing the ids for parts oft he compass which are actually used in the CSS to hide/change depending on the state. This PR adds those ids back.
@coliff coliff mentioned this pull request May 29, 2024
8 tasks
HarelM added a commit that referenced this pull request May 30, 2024
I introduced a bug with #4181 by removing the ids for parts oft he compass which are actually used in the CSS to hide/change depending on the state. This PR adds those ids back.

Co-authored-by: Harel M <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants