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

Batch optimize SVGs #1275

Merged
merged 10 commits into from Mar 28, 2019

Conversation

4 participants
@ericcornelissen
Copy link
Member

commented Mar 8, 2019

Batch optimize all icons present in the repository (if applicable) using this SVGO config (zip):

  • In light of #1272 I compounded all icons in c3c426f (99 additions changed, see ⬇️)
  • Removed newlines at the end of files and trailing spaces in SVGs in 0f536e5 (trivial changes, no review required)
  • Converted shapes to <path> (and also compounded the <path>) in f711cea (6 files changed)

Compounding <path>s

This is a pain to review manually, but I went through all icons and I think everything is okay except for the ones listed here. Pinging @goyney before trying to resolve this myself...

@ericcornelissen ericcornelissen referenced this pull request Mar 8, 2019

Open

Return raw path data in NPM #1272

2 of 2 tasks complete
@davidklebanoff

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

This is good stuff! Thanks @ericcornelissen and @goyney. I can review them this weekend as well. Thankfully the onion skin makes it somewhat manageable.

@goyney

This comment has been minimized.

Copy link

commented Mar 8, 2019

@ericcornelissen Not sure what's happening in those specific cases. I'm not too familiar with the internal workings of SVGO, but my guess is it is not perfect. :) I'll see if I can find some time this weekend to look into it as well.

@davidklebanoff

This comment was marked as resolved.

Copy link
Member

commented Mar 11, 2019

I reviewed all the icons, I found issues with these as well:

  • Automatic
  • Figma
  • Laravel
@ericcornelissen

This comment was marked as resolved.

Copy link
Member Author

commented Mar 12, 2019

Except for Laravel (which I added to the PR description) I'm not sure what issues you found @davidklebanoff, I compared c3c426f to develop:

Automatic

automatic

Figma

figma

@ericcornelissen ericcornelissen marked this pull request as ready for review Mar 12, 2019

@ericcornelissen

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

When this gets merged, we should make sure #1277 is implemented as soon as possible as well so a PR like this isn't needed in the future

@davidklebanoff

This comment was marked as resolved.

Copy link
Member

commented Mar 12, 2019

@ericcornelissen - This is all very strange/concerning. I see the issues on those icons in GitHub's "diff" view, but the issues are not showing when I view the icons directly on your branch. It may just be an issue on GitHub's display side - very odd and concerning all the same. I'll see if I can reproduce the issue elsewhere.

I'm attaching screenshots below.

Automatic - note the top of the A:
image

Figma:
image

@ericcornelissen

This comment was marked as resolved.

Copy link
Member Author

commented Mar 13, 2019

Hmm @davidklebanoff, that is concerning and it is weird that the issue looks so much different from the other issues 🤔

What browser are you using? I'm not having this issue in Firefox nor Chrome. Also, what happens if you just download the (updated) icons and try to preview them?

@davidklebanoff

This comment was marked as resolved.

Copy link
Member

commented Mar 14, 2019

I'm on Mac OS. Testing the Figma icon I can reproduce the issue using the GitHub diffing view in Chrome, but only there. If I view the icon itself in Chrome (not through the diffing tool) it looks fine. It also looks ok downloaded locally - viewed in the file browser, and in the GitHub diffing view in Safari and Firefox.

I don't know why the issue is occuring, but seeing as it's only occurring in the diffing view for GitHub, and only in one browser at that, I suspect it may be an issue with GitHub's diffing tool and not the icon itself.

@ericcornelissen

This comment was marked as resolved.

Copy link
Member Author

commented Mar 14, 2019

That is peculiar @davidklebanoff... Just for the record, what version of Chrome are you running?

That said, I don't think it is worth our time to investigate this further, or at least prevent this PR from being merged because of it...

@davidklebanoff

This comment was marked as resolved.

Copy link
Member

commented Mar 14, 2019

Chrome Version 71.0.3578.98 (Official Build) (64-bit)

@ericcornelissen ericcornelissen referenced this pull request Mar 15, 2019

Merged

Adding Reason SVG #1290

3 of 3 tasks complete
@metaa

This comment was marked as off-topic.

Copy link
Contributor

commented Mar 24, 2019

I actually just started trying to get a script to work that would automatically manipulate SVG files in a way that they're exactly what we want them to be (just like the config linked here), just SVGO's plugins don't seem to work for me and that's pretty much the only library I found to be able to do the stuff we need so far 🤔

@ericcornelissen

This comment was marked as off-topic.

Copy link
Member Author

commented Mar 25, 2019

I actually just started trying to get a script to work that would automatically manipulate SVG files in a way that they're exactly what we want them to be (just like the config linked here), just SVGO's plugins don't seem to work for me and that's pretty much the only library I found to be able to do the stuff we need so far 🤔

What problem exactly are you having @metaa?

Also, ideally this would just be a configuration file that can be loaded into SVGOMG so that contributors don't have to download anything else than a simple configuration file (i.e. nothing that they need to run on their machine). But for the time being any alternative to that is appreciated 😄

@metaa

This comment was marked as off-topic.

Copy link
Contributor

commented Mar 25, 2019

What problem exactly are you having @metaa?

I played around with SVGO but no matter what plugins I enabled or disabled, the output was always the same 🤔

My goal was to craft a script that resizes the icon and viewbox, centers the icon, merges paths & so on (basically what the contribution guide asks for).

@ericcornelissen

This comment was marked as off-topic.

Copy link
Member Author

commented Mar 25, 2019

The command below ⬇️ works for me (assuming both the SVGO repo and simple-icons repo share the same parent, and you installed the dependencies for SVGO) can you test if it removes the double <path> in the laravel SVG on develop given the config in the PR description? If that works you should be able to enable/disable plugins in the config and everything should work fine.

$ node .\svgo\bin\svgo .\simple-icons\icons\laravel.svg -o ./_tmp.svg --config=.\svgo\.svgo.yml

That said, as pointed out by @goyney "I'm not too familiar with the internal workings of SVGO, but my guess is it is not perfect. :)", it might just be that what you want to achieve doesn't work perfectly in SVGO...

@metaa

This comment was marked as off-topic.

Copy link
Contributor

commented Mar 25, 2019

can you test if it removes the double <path> in the laravel SVG on develop given the config in the PR description?

Doesn't change, no.

@ericcornelissen

This comment was marked as off-topic.

Copy link
Member Author

commented Mar 27, 2019

Not sure what is going on @metaa, if you want you can set up a (temporary) repo or gist so you can share your setup and I will have a look if I can get it working 👍

@davidklebanoff davidklebanoff merged commit e050634 into simple-icons:develop Mar 28, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davidklebanoff

This comment has been minimized.

Copy link
Member

commented Mar 28, 2019

I've gone ahead and merged this in. Thanks for the cleanup effort @ericcornelissen!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.