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

Optimised images. #4573

Merged
merged 1 commit into from
Dec 1, 2017
Merged

Optimised images. #4573

merged 1 commit into from
Dec 1, 2017

Conversation

grischard
Copy link
Contributor

Saved 91 KB out of 762 KB. 20.5% per file on average (up to 56.8%).

For every file, ImageOptim compared the different tools and picked the smallest file that was produced by a combination of advpng, pngout and zopfli for PNGs, svgo for SVGs and gifsicle for GIFs.

No related issues, no code change, no functionality change, no visual differences in images (compression is lossless).

@bhousel
Copy link
Member

bhousel commented Nov 29, 2017

This seems ok, but a bunch of these images aren't used anymore and we should just remove them anyway. Some thoughts:

  • The 2x cursors don't seem to be used currently. (we should fix that)
  • Really the only things that matter for optimization are the assets in dist/
  • Everything under docs/ is very old and needs to go away or be redone
  • The one svg/iD-sprite.src.svg file definitely does not need to be optimized, because it's an intermediate step generated file. It's later used to generate a different file which contains <symbol> elements in dist/img/iD-sprite.svg as part of the build process
  • We've had issues with svgo optimizing away our symbol spritesheet. I opened an issue Fills are stripped from elements inside a <symbol> svg/svgo#743 - anyway just be careful using svgo, some of the rules are disabled on openstreetmap-website because of this bug.

Can you change this PR to be just the images under dist/?

@grischard
Copy link
Contributor Author

Happy to! I'll try to get around to it today.

@grischard
Copy link
Contributor Author

Couldn't most of those images in dist/ be SVGs? Do we still have the vector source for them somewhere?

@bhousel
Copy link
Member

bhousel commented Nov 29, 2017

Couldn't most of those images in dist/ be SVGs? Do we still have the vector source for them somewhere?

We already went through in #2785 and moved most of the image assets into the SVG spritesheet.

I think that covers everything!

@grischard
Copy link
Contributor Author

https://github.com/yangshun/awesome-spinners has some cool non-gif spinners, and the current spinner isn't optimised for retina. Maybe I'll take that as a project for a hacking weekend :).

Could this be merged now as it is?

@bhousel
Copy link
Member

bhousel commented Nov 29, 2017

We can't switch animated objects like the loading spinners

Another thought on this: technically, we can but it's not a drop in replacement, and probably not worth it to redo the image and wire up a css transform and sort out all the cross browser issues, just to have a loading spinner that takes up a bit less space and looks a little nicer on retina displays. If you want to ImageOptim them though, that's cool.

@bhousel
Copy link
Member

bhousel commented Nov 29, 2017

https://github.com/yangshun/awesome-spinners has some cool non-gif spinners, and the current spinner isn't optimised for retina. Maybe I'll take that as a project for a hacking weekend :).

Cool, yeah if you really want to do this, I won't say no 👍

Watch out for <animateTransform> which isn't supported on IE

@bhousel
Copy link
Member

bhousel commented Dec 1, 2017

I took a look and this seems fine, thanks @grischard 👍

@bhousel bhousel merged commit d310cee into openstreetmap:master Dec 1, 2017
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.

2 participants