Skip to content

Conversation

@grischard
Copy link
Contributor

@grischard grischard commented Nov 22, 2017

Saved 12.5 MB out of 56.7 MB. 27.5% per file on average (up to 99.6%)

Description:

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

This includes a rather ironic 74.8% change on source/images/blog/2016-08-optimizing-web-app/performance-diagram.png which went from 38237 bytes to 9620 bytes.

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

Copy link
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Images from third-parties (plugins, themes, etc) should be optimized in their source otherwise those images needs to be optimized again when we update. Thus I would limit it to source/images/ source/demo/images/ and source/static/images/.

@grischard
Copy link
Contributor Author

That makes sense. There's a few stray images in source/demo/ and source/favicon.png and source/static/ too, and it looks like the only two third-party sources are jwplayer/jwplayer and octopress/classic-theme. I'll submit separate PRs there, and update the PR here.

@grischard
Copy link
Contributor Author

I actually can't find the source for the jwplayer glow theme. It looks like those files haven't been touched since 2014. What do you think we should do?

@grischard
Copy link
Contributor Author

So both jwplayer/jwplayer#2541 and octopress/classic-theme#6 have been opened, and haven't received any attention :). I'm happy to re-optimise the images and submit a new PR here if and when we update those if upstream ignores them.

@fabaff do you see this as blocking to merge this PR, or could it be merged like that?

Copy link
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Let's merge it.

Thanks 🐦

@fabaff fabaff merged commit 9454a5f into home-assistant:next Nov 27, 2017
@grischard
Copy link
Contributor Author

Thanks!

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