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

add spring icon #1706 #1822

Merged
merged 1 commit into from Oct 24, 2019
Merged

add spring icon #1706 #1822

merged 1 commit into from Oct 24, 2019

Conversation

bgalek
Copy link
Contributor

@bgalek bgalek commented Oct 24, 2019

Issue:
#1706 add spring icon (spring.io)

Checklist

  • I updated the JSON data in _data/simple-icons.json
  • I optimized the icon with SVGO or SVGOMG
  • The SVG viewbox is 0 0 24 24

Description

@ericcornelissen ericcornelissen added the new icon Issues or pull requests for adding a new icon label Oct 24, 2019
@birjj
Copy link
Contributor

birjj commented Oct 24, 2019

Hey @bgalek, thanks a lot for the PR! A few things we'd like changed before we can merge this in:

  • We require that each icon has a <title> containing the name of the brand followed by "icon" (in this case "Spring icon"). This is for a11y reasons.
  • It looks like there's an extra fill-rule="nonzero" at the end of your <path>. Could you remove it? From my testing I don't think it should break anything to simply delete it.
  • We try to keep the icon files as small as possible. Could you remove the extra whitespace (namely newlines)?
  • Could you explain a bit about where you found the SVG? I can't seem to find an official SVG at https://spring.io/trademarks

@bgalek
Copy link
Contributor Author

bgalek commented Oct 24, 2019

Hi @birjolaxew!
I've added title (I missed it out when reading CONTRIBUTING.md) - sorry ;) Maybe it should be added to pull request checklist? :)
I've removed the fill-rule and new lines.
I've seperated and vectorized this file from https://spring.io/img/spring-by-pivotal-9066b55828deb3c10e27e609af322c40.png available at https://spring.io/trademarks :)

@bgalek bgalek closed this Oct 24, 2019
@bgalek bgalek deleted the spring-icon branch October 24, 2019 21:17
@bgalek bgalek restored the spring-icon branch October 24, 2019 21:17
@birjj birjj reopened this Oct 24, 2019
@birjj
Copy link
Contributor

birjj commented Oct 24, 2019

Thanks a lot for the update!

The checklist in our PR template doesn't quite cover everything our contribution guidelines do; we used to have a lot of new contributors that either submitted SVGs directly from the source, without minifying/resizing it, or didn't update the JSON correctly - the checklist is mostly used as a reminder to contributors that we have some formatting requirements, even if it doesn't cover all of them (although it might be time to update it ;) ). Thanks a lot for reading the contribution guidelines regardless! That does mean a lot to us.

One last thing that I apparently missed in my last comment: I'm getting #6DB33F when I color pick the logo, while this PR uses #77BC1F - although the difference is obviously pretty small, we prefer to use official colors where possible. Did you pick your color from somewhere else?

@bgalek
Copy link
Contributor Author

bgalek commented Oct 24, 2019

It's possible that I messed with it during vectorization process, I'll update the color! :)

@birjj
Copy link
Contributor

birjj commented Oct 24, 2019

👍 That is all the nitpicking I could do this time around I think ;) The vectorization looks excellent. Thanks a lot for the updates and all your work on this @bgalek!

@birjj birjj merged commit 6d653fc into simple-icons:develop Oct 24, 2019
@birjj birjj mentioned this pull request Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new icon Issues or pull requests for adding a new icon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants