Skip to content
This repository has been archived by the owner. It is now read-only.

Fixing #2536 : Optimized the Github SVG on the Get Involved page to display properly in Firefox #2537

Merged
merged 1 commit into from Nov 16, 2017

Conversation

@marcobeltempo
Copy link
Contributor

@marcobeltempo marcobeltempo commented Oct 21, 2017

Fixing #2536

Issue

The Github icon on https://thimble.mozilla.org/en-US/get-involved/ was displaying in black when it should have been white.
This only occurred using Firefox.

image

Fix

As per @flukeout suggestion, I ran public/resources/img/icon/github-white.svg through https://jakearchibald.github.io/svgomg/ to optimize the SVG

Tested in

  • Firefox 57
  • Chrome 61
  • Edge 41

Result

image

@humphd humphd requested a review from flukeout Oct 21, 2017
Copy link
Contributor

@gideonthomas gideonthomas left a comment

Hi @marcobeltempo, sorry for the delayed response!

Would you be able to use:

<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 32.58 31.77"><title>github-white</title><path d="M16.29 0a16.29 16.29 0 0 0-5.15 31.75c.81.15 1.11-.35 1.11-.79v-2.77C7.7 29.18 6.74 26 6.74 26a4.31 4.31 0 0 0-1.81-2.38c-1.48-1 .11-1 .11-1a3.42 3.42 0 0 1 2.5 1.68 3.47 3.47 0 0 0 4.74 1.35 3.48 3.48 0 0 1 1-2.18c-3.58-.39-7.38-1.79-7.38-8.03a6.3 6.3 0 0 1 1.68-4.37 5.86 5.86 0 0 1 .16-4.31s1.37-.44 4.48 1.67a15.44 15.44 0 0 1 8.16 0c3.11-2.11 4.48-1.67 4.48-1.67a5.85 5.85 0 0 1 .14 4.31 6.29 6.29 0 0 1 1.67 4.37c0 6.26-3.81 7.63-7.44 8a3.89 3.89 0 0 1 1.11 3v4.47s.29.94 1.12.78A16.29 16.29 0 0 0 16.29 0z" fill="#fff" fill-rule="evenodd" data-name="Layer 1"/></svg>

instead? It's the same as yours except it's minified (no spaces, new lines, etc.) since we don't automatically minify svgs, html, etc.

@marcobeltempo
Copy link
Contributor Author

@marcobeltempo marcobeltempo commented Nov 6, 2017

Thanks for the feedback @gideonthomas! I've applied the recommended changes

@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 7, 2017

@marcobeltempo, looks like you might have updated the wrong file. The change should be made to github-white.svg instead of gear-white.svg in the public/resources/img/icon folder.

@marcobeltempo marcobeltempo force-pushed the marcobeltempo:issue2536 branch from 692734f to 31fc64a Nov 13, 2017
@marcobeltempo
Copy link
Contributor Author

@marcobeltempo marcobeltempo commented Nov 13, 2017

@gideonthomas I apologize fore the delay and the mix up. I applied the proper changes

@marcobeltempo marcobeltempo force-pushed the marcobeltempo:issue2536 branch from 31fc64a to 0df857b Nov 13, 2017
@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 14, 2017

hmm that is weird! @marcobeltempo can you try rebasing your patch onto what's on master and pushing that up. Hopefully that will fix Appveyor

@marcobeltempo marcobeltempo force-pushed the marcobeltempo:issue2536 branch 2 times, most recently from a67f354 to eca8555 Nov 14, 2017
@marcobeltempo
Copy link
Contributor Author

@marcobeltempo marcobeltempo commented Nov 14, 2017

@gideonthomas I tried rebasing but still no luck

@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 15, 2017

hmm, this is so bizzare! @cadecairos any idea what's going on here? The eslint prettier rules that appveyor claims to be violated are firstly, insane, and secondly not caused by this PR.

@cadecairos
Copy link
Contributor

@cadecairos cadecairos commented Nov 15, 2017

This is odd. I've restarted the build... maybe there's a problem with a dependency that had a bad patch update.

@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 15, 2017

Wat...now Travis is failing too

@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 15, 2017

aha! You're right @cadecairos, looks like a minor version bump in prettier broke stuff. I'll fix this.

@gideonthomas
Copy link
Contributor

@gideonthomas gideonthomas commented Nov 15, 2017

@marcobeltempo, sorry about that. Looks like one of the dependencies was updated and it broke stuff. I've fixed this on master, so if you rebase again onto master, it should work.

@marcobeltempo marcobeltempo force-pushed the marcobeltempo:issue2536 branch from eca8555 to 048bc52 Nov 15, 2017
@marcobeltempo
Copy link
Contributor Author

@marcobeltempo marcobeltempo commented Nov 15, 2017

@gideonthomas nice! looks to have done the trick

Copy link
Contributor

@gideonthomas gideonthomas left a comment

This works great! Thanks @marcobeltempo! Once again sorry about the appveyor issues.

@gideonthomas gideonthomas merged commit 8efcf9a into mozilla:master Nov 16, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marcobeltempo marcobeltempo deleted the marcobeltempo:issue2536 branch Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants