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

Adding files for SaltStack Icon #1835

Merged
merged 3 commits into from Oct 29, 2019
Merged

Conversation

kaleeaswari
Copy link
Contributor

@kaleeaswari kaleeaswari commented Oct 26, 2019

Issue: #1809

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

Source for the icon: https://www.saltstack.com/resources/brand/

@phatbhoy67
Copy link
Contributor

phatbhoy67 commented Oct 26, 2019

Hey @kaleeaswari thanks for working on this PR. 👍.

A few changes are required.

Please include in the "description" the source you used for the .svg and the reason you chose the hex value #00EACE.

Is there a reason you included the ® symbol?

Some changes are required for the .svg file.

  • The icon is not centered in the view box.

  • The icon is larger than 24px tall.

In the image below, the grey square represents the 24 x 24px viewbox if I center your icon.

saltstack

Alexa Rank: 198,466

@phatbhoy67 phatbhoy67 added changes requested new icon Issues or pull requests for adding a new icon labels Oct 26, 2019
@kaleeaswari
Copy link
Contributor Author

Hi @phatbhoy67,

Source for the icon: https://www.saltstack.com/resources/brand/
And list of colors are also mentioned in the same web page.
And the R symbol is also mentioned in the same web page as part of icon.

@phatbhoy67
Copy link
Contributor

phatbhoy67 commented Oct 27, 2019

Hey @kaleeaswari thanks for updating the icon and the information 👍.

Looking at the .svg, it requires a further adjustment. Some of the icon is outside the viewbox.

If you open the file in Inkscape, select the icon, lock the aspect ratio and then set the height to 24px. Then center it again horizontally and vertically on the page. That should fix it. See below for how it looks now.

scrnli_27_10_2019_15-04-16

I've added the source to the description, I also added the issue reference number this PR looks to address. You can reference an issue by typing its number e.g. #1835 with a # at the start which will auto link back to the issue. 🎉

@phatbhoy67 phatbhoy67 requested a review from birjj October 28, 2019 12:25
@phatbhoy67
Copy link
Contributor

Hey @kaleeaswari thanks for making the changes requested 👍, I've requested a review, hopefully this will be merged soon! 👏

@birjj
Copy link
Contributor

birjj commented Oct 29, 2019

👍 Looks good! Thanks a lot for your work on this @kaleeaswari, and for your thorough review @phatbhoy67!

On a sidenote, you're free to merge it yourself if you're confident the PR is good @phatbhoy67 - you're a maintainer on par with all the rest of us ;)

@birjj birjj merged commit 0396ccf into simple-icons:develop Oct 29, 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

4 participants