Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

SVG optimisation; the return #774

Merged
merged 1 commit into from Feb 27, 2015

Conversation

Projects
None yet
3 participants
Contributor

louisjc commented Feb 25, 2015

As suggested by @saivann in bitcoin#726, I've optimized every icon SVG file. I've used this batch tool with non destructive options plus some manual fixing. The images should be visually identical and according to my tests on firefox, they are (double check may be required). They are now 3 times smaller, it should save some bandwidth.

NB: For futures images, the Javascript version should be enough.

@louisjc louisjc changed the title from SVG optimisation the return to SVG optimisation; the return Feb 25, 2015

Contributor

harding commented Feb 25, 2015

According to my tests, the optimized images save 251,227 bytes in disk usage. Here's is a single image of all the current SVGs from my browser:

2015-02-25-121417_688x652_scrot

And here is an image of all the optimized images:

2015-02-25-121347_695x654_scrot

The only differences I see are a slight difference in color tone in the RSS icon and the !-inside-triangle warning images. I don't think that matters, so this pull LGTM. Thanks for another great PR, @louisjc!

Contributor

saivann commented Feb 25, 2015

@louisjc Thanks, great work!

There is some compatibility issues with Safari 4, the screenshot below shows how many icons on the https://bitcoin.org/en/innovation page are rendering (some parts of the icons do now show). I do not have higher Safari versions to test how widespread this issue would be yet, any idea of a way of fixing this? Or does anyone have time to test higher versions of Safari?

capture du 2015-02-25 13 45 22

Contributor

louisjc commented Feb 25, 2015

@saivann It works fine with Safari 8 on OS X 10.10.
I would like to debug it but I think there is no way to get an older Safari...

Contributor

saivann commented Feb 25, 2015

@louisjc Thanks for your additional testing! I've found a Safari 5.0 version and couldn't reproduce this issue with it either, so I'd say it probably affects an almost inexistent number of browsers. LGTM!

Contributor

harding commented Feb 25, 2015

Thanks both @louisjc and @saivann for your additional testing.

In the absence of critical feedback, this will be merged around 22:00 UTC Thursday.

Contributor

louisjc commented Feb 26, 2015

@harding @saivann How about add Safari 4 in the fallbackSVG ?
Safari 4 don't seems to support window.SVGSVGElement, so :

function supportsSVG() {
...
+ if (!(window.SVGSVGElement)) return false;
...
}

(Here) Should do it. Again I can't test it on Safari 4, all i can say is that it only trigger on IE version already under the fallback, and do not trigger on recent FF, Safari and Chrome.

EDIT: according to this it should be the best way to do it as it only trigger the browsers we want under the svgfallback.

Contributor

saivann commented Feb 26, 2015

If Safari 4 is the only affected browser, I suggest we postpone the javascript improvement for when we'll need to make other changes to base.js, since editing base.js will trigger an alert for the people monitoring the download page. Safari 4 is virtually extinct with 0.0% of visitors according to each stats I've checked.

Contributor

harding commented Feb 26, 2015

@saivann it's probably worth creating a branch like bitcoin/minor-breaking-changes where we can put stuff like this until we have a major change, so that we don't forget the minor stuff and can merge it in together with the major stuff.

Anyway, based on @saivann's comment, I continue to plan to merge this PR as-is around 22:00 UTC today.

@harding harding merged commit 8b1a9d8 into bitcoin-dot-org:master Feb 27, 2015

harding added a commit that referenced this pull request Feb 27, 2015

Merge pulls #774 and #776
* #774: SVG optimisation; the return
* #776: QA: Simplify Comparison Between Local And Remote Site Files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment