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 Istio icon #6093

Merged
merged 9 commits into from
Jul 18, 2021
Merged

Add Istio icon #6093

merged 9 commits into from
Jul 18, 2021

Conversation

swarup4741
Copy link
Contributor

@swarup4741 swarup4741 commented Jul 9, 2021

istio

Issue: #6090
Alexa rank: 88,863

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
  • tests performed
  • linting performed

Description

added the istio icon and added the metadata to _data/simple-icons.json with hex value 466AB1 as provided in issue #6090 and also fixed some linting issues as well.

@github-actions github-actions bot added the new icon Issues or pull requests for adding a new icon label Jul 9, 2021
@service-paradis service-paradis changed the title Added icon Add Istio icon Jul 10, 2021
@service-paradis service-paradis linked an issue Jul 10, 2021 that may be closed by this pull request
Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SVG looks spot on (even with this low precision), nice work @swarup4741!

Before we can merge this in though, I have two requests:

  • Can you revert the change the package-lock.json?
  • If you used the SVG linked in the issue as source material, could you update the "source" to "https://github.com/istio/istio/blob/5a047251817eb2523af297607b7614120812e47a/logo/istio-bluelogo-whitebackground-unframed.svg"? Else, could you elaborate on where exactly on the website you found the SVG and why you chose it over this one?

@ericcornelissen
Copy link
Contributor

Thank you for updating the source @swarup4741. However, it looks like the changes to package-lock.json are still there, it might be easiest to just copy the content from the package-lock.json on the develop branch and pasting it to the file on your branch (the changes are due to you using npm v7, and this project still using a lockfile generated by npm <7).

Also, I didn't catch this the first time around, but where exactly did you get the color hex value from? The SVG in the source has #466BB0, not #466AB1 🤔

@swarup4741
Copy link
Contributor Author

Thank you for updating the source @swarup4741. However, it looks like the changes to package-lock.json are still there, it might be easiest to just copy the content from the package-lock.json on the develop branch and pasting it to the file on your branch (the changes are due to you using npm v7, and this project still using a lockfile generated by npm <7).

Also, I didn't catch this the first time around, but where exactly did you get the color hex value from? The SVG in the source has #466BB0, not #466AB1 🤔

I got the hex the value by using a color picker tool... shall I update the hex value ?

@ericcornelissen
Copy link
Contributor

I got the hex the value by using a color picker tool... shall I update the hex value ?

Using the color picker tool on the source SVG? Or on something else? From the source SVG I'm getting #466BB0 with the color picker as well. If you got the brand color from somewhere else that could be valid as well. I'm not at all familiar with the brand, but it seems blue is the brand color and I'm not seeing any specific color they prefer so running the value in the SVG seems as good as any. If you're familiar with the brand though, feel free to argue for any color you think makes sense 🙂

@swarup4741
Copy link
Contributor Author

swarup4741 commented Jul 17, 2021

I got the hex the value by using a color picker tool... shall I update the hex value ?

Using the color picker tool on the source SVG? Or on something else? From the source SVG I'm getting #466BB0 with the color picker as well. If you got the brand color from somewhere else that could be valid as well. I'm not at all familiar with the brand, but it seems blue is the brand color and I'm not seeing any specific color they prefer so running the value in the SVG seems as good as any. If you're familiar with the brand though, feel free to argue for any color you think makes sense 🙂

No more worries.. i updated the hex to #466BB0. Everything is ok now i suppose ...

@ericcornelissen
Copy link
Contributor

In that case the hex value seems fine (it's always possible to change this later if someone disagrees) - the only thing left to do is revert the changes of the package-lock.json file. If you need help with that, I can do it for you before and merge it in immediately after that.

As an aside, there's no need to merge develop in whenever the option appears on your Pull Request. Generally, not much changes on the develop that will affect your work. The reason we require the branch to be up-to-date with develop at this point in time is because there are still some old Pull Requests that we know of that may require some changes in order to be merged. In any case, we will take care of merging develop into your branch when it's ready to merge for you 🙂

@swarup4741
Copy link
Contributor Author

In that case the hex value seems fine (it's always possible to change this later if someone disagrees) - the only thing left to do is revert the changes of the package-lock.json file. If you need help with that, I can do it for you before and merge it in immediately after that.

As an aside, there's no need to merge develop in whenever the option appears on your Pull Request. Generally, not much changes on the develop that will affect your work. The reason we require the branch to be up-to-date with develop at this point in time is because there are still some old Pull Requests that we know of that may require some changes in order to be merged. In any case, we will take care of merging develop into your branch when it's ready to merge for you 🙂

I have changed the package-lock.json file as per you said.. just copied the whole from develop and pasted it in my package-lock.json file in my branch and pushed it

@ericcornelissen
Copy link
Contributor

I have changed the package-lock.json file as per you said.. just copied the whole from develop and pasted it in my package-lock.json file in my branch and pushed it

My apologies, the GitHub interface says there's 3 changed files so I assumed you didn't change anything yet 😅 Though you did revert most of the changes, you did end up changing (somehow) some of the whitespace in that file. Could you revert those changes as well? (Just to keep our commit history clean.)

Copy link
Contributor Author

@swarup4741 swarup4741 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the white-spaces....

@ericcornelissen
Copy link
Contributor

removed the white-spaces....

As you can see in the GitHub UI there are still changes between package-lock.json in your branch and on develop...

If you want I can take care of this and merge this in for you 🙂 If you want to give it another try, make sure you are copying the content of package-lock.json from the most recent commit on develop, e.g. this one.

@swarup4741
Copy link
Contributor Author

removed the white-spaces....

As you can see in the GitHub UI there are still changes between package-lock.json in your branch and on develop...

If you want I can take care of this and merge this in for you 🙂 If you want to give it another try, make sure you are copying the content of package-lock.json from the most recent commit on develop, e.g. this one.

I’ll give it another try … if this time if i’m unable to fix it (somehow) then you please kindly take over and make it happen … I’ll make the changes tomorrow.. (thank you for guiding so much … I’m learning a lot)

Copy link
Contributor

@ericcornelissen ericcornelissen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ll give it another try … if this time if i’m unable to fix it (somehow) then you please kindly take over and make it happen … I’ll make the changes tomorrow.. (thank you for guiding so much … I’m learning a lot)

Almost did it, just one final newline missing in package-lock.json - fixed that one for you 🙂

Thanks for working on this one @swarup4741 🎉

@ericcornelissen ericcornelissen merged commit bcc22e8 into simple-icons:develop Jul 18, 2021
dirien pushed a commit to dirien/simple-icons that referenced this pull request Jul 20, 2021
* Added a new icon

* Updated the hex value
ericcornelissen added a commit that referenced this pull request Jul 25, 2021
# New Icons

- DPD (#5831)
- Dungeons &amp; Dragons (#5837)
- Grab (#6086)
- Istio (#6093)
- Packer (#5649)
- Windi CSS (#6103)

# Updated Icons

- arXiv (#5884)
- Douban (#6114)
- Douban Read (#6114)
- S7 Airlines (#6147)
- Salesforce (#6147)
- Samsung (#6147)
- Samsung Pay (#6147)
- Sass (#6147)
- Scaleway (#6147)
- Scania (#6147)
- Scribd (#6147)
- Sencha (#6147)
- SEPA (#6147)
- Shazam (#6147)
- Shopify (#6147)
- Shutterstock (#6147)
- Sketch (#6147)
- Slack (#6147)
- SmartThings (#6147)
- Snapchat (#6147)
- Snapcraft (#6147)
- Snowflake (#6147)
- Solidity (#6147)
- SonarLint (#6147)
- SonarQube (#6147)
- SonarSource (#6147)
- Songkick (#6147)
- Songoda (#6147)
- Sourcegraph (#6147)
- Spacemacs (#6147)
- SparkPost (#6147)
- Spotify (#6147)
- Spreaker (#6147)
- Squarespace (#6147)
- Stackbit (#6147)
- StackPath (#6147)
- Starling Bank (#6147)
- STARZ (#6147)
- Statamic (#6147)
- Steam (#6147)
- Steinberg (#6147)
- Stellar (#6147)
- Streamlit (#6147)
- Stylus (#6147)
- Sublime Text (#6147)
- Sumo Logic (#6147)
- SurveyMonkey (#6147)
- SUSE (#6147)
- SVG (#6147)
- Symfony (#6147)
- Synology (#6147)
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.

Add Istio
2 participants