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

Added new simple icons and updated tor.svg #1834

Closed
wants to merge 2 commits into from

Conversation

tansiret
Copy link

@tansiret tansiret commented Oct 25, 2019

Issue:

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

@foo-dogsquared
Copy link
Contributor

Hello @Yutyo ! It would be more appreciated if you make individual branches for each icon so that the maintainers can easily check whether the brand you've added/updated is valid (especially that you have ten icons to begin with).

Please refer to the contributing guidelines for more details.

While we're at the discussion and waiting for the maintainers' feedback, here's some quick feedback on the SVGs:

  • While your icons look good (at least to me), most of them haven't touched at least two edges (horizontally or vertically).
  • The SVGs are not optimized and consist of multiple <path>s.
  • Some of the icons (such as the Audacity icon) have a subtle shadow on them. Please remove it.

The resulting SVGs indicate that you're using Inkscape.
I'm open to help (though I just started using it this month) if you have trouble with it.

Regarding whether the brands is valid when compared against the criteria, Mercode unfortunately does not fit.
If there's any other indicator of popularity for your brand, you can add it as a note.

Lastly, the JSON is not updated with the information of the added/updated icons. This would be helpful in knowing the source of the icons. The official site and the API also relies on the JSON so it is mandatory to update it.

@tansiret
Copy link
Author

OK. So would it be better if I close this pull request and open a new one after updating?

Mercode is not popular as a brand but has a Hubzilla fork named as Merhub (hub.mercode.org) which is way better than GNUSocial and Diaspora. I am gonna add Hubzilla icon too.

@foo-dogsquared
Copy link
Contributor

I don't think it is needed now since it'll be merged as one consistent commit. Just make sure to create individual pull requests per icon next time. :)

@ericcornelissen ericcornelissen added the new icon Issues or pull requests for adding a new icon label Oct 26, 2019
@tansiret
Copy link
Author

I have pushed a new updated according to your recommendations and added some more icons but haven't edited JSON yet. Do you talk about simple_icons.json or all JSON files? I ask because it needs to pull the icons from icons folder, right? I can't give source for all because half of those are designed by me, not directly copied and painted to black.

Lastly, the JSON is not updated with the information of the added/updated icons. This would be helpful in knowing the source of the icons. The official site and the API also relies on the JSON so it is mandatory to update it.

@foo-dogsquared
Copy link
Contributor

The JSON I'm referring to is the simple_icons.json.

The source doesn't have to point to the SVG itself.
It could be the favicon of the site, a picture from a Wikipedia article, any sources (well, only one source anyway) you've used to create the icon will do.

@ericcornelissen
Copy link
Contributor

Hi @Yutyo please read through the Contributing Guidelines' Adding or Updating an icon section and update the SVGs in this Pull Request accordingly.

As for the JSON you can read more about that in the Contributing Guidelines as well, the update the JSON data section. Essentially you need to add some metadata (i.e. information about the brand) in the _data/simple-icons.json file.

OK. So would it be better if I close this pull request and open a new one after updating?

That would be better for any future contributions you make, but for the icons that are already in this Pull Request it is fine.

[...] designed by me [...]

Can you elaborate? We only accept icons that the brand itself uses as well. E.g. I see you updated the SVG for Tor, but the Tor website does not seem to use the icon? Did you design it yourself as well?


If you have any more questions feel free to ask them here.

Thanks for the early feedback @foo-dogsquared 👍

@tansiret
Copy link
Author

tansiret commented Oct 27, 2019

They aren't invented by me but some of them are just got minimalised/ revised such as GnuCash and Counter Strike. Tor logo itself is the one being used in the new browser.

Also I think that svgs I made are according to your guidelines. They touch the edges and are monochrome. I couldn't cut the white sectors on Retroarch and Tor logo as Inkscape gave several errors for some unknown reason so it would be cool if you can cut those parts.

@birjj
Copy link
Contributor

birjj commented Oct 27, 2019

Hi @Yutyo. I'll just quickly sum up the changes we'd like to see to this PR before we can merge, as described in our contribution guidelines. This is unfortunately going to take a bit of work because there are quite a few icons in this PR, but I'm hoping this list will make everything clear, at the very least:

  • We use _data/simple-icons.json to keep track of some metadata about each icon. This includes the brand name, what its main color is, as well as where the icon was taken from (this should be as official as possible). You should add information about each icon in there. More information here

  • Some of the SVGs are not quite monochrome, e.g. ipfs.svg and retroarch.svg have white paths.

  • Some of the icons seem to be slightly offset from the center, meaning they are cut off at one side or another. Could you make sure that each icon is 24pt x 24pt, and has its center at (12pt, 12pt)?

  • As pointed out by @foo-dogsquared, Mercode unfortunately isn't quite big enough of a brand for us to add. Could you remove it from the PR?

  • Some of the icons don't appear to be the official one. GnuCash looks to be the wrong icon entirely, and Audacity, Counter-Strike and OpenShot differ significantly from their official version. RetroPie also looks like it could be monochromized in a way that better shows off the buttons and joystick.

  • Some of the icons don't have very accurate brand names. The Tor icon is actually the new icon for the Tor Browser (the Tor Project itself still uses the old icon), and Counter-Strike should probably be renamed "Counter-Strike: Global Offensive" and updated to the official logo (or renamed to one of the other versions, and use their official logo)

  • It looks like all your SVGs are exported straight from Inkscape. This gives some pretty huge file sizes that we want to bring down a bit. Can you run each SVG through SVGOMG to remove all the unnecessary stuff, annotate them, and also merge together all the <path>'s (if there are multiple)?

I know that it will take a lot of work to update all the icons in this PR; you are free to choose to remove some if you'd like to lessen the workload. In the future, it might be helpful to start off small if you are unsure of the process - we'll be happy to help if you have any questions!

@birjj birjj closed this Nov 26, 2019
@birjj birjj added the abandoned Pull requests that have been abandoned by the contributor label Nov 26, 2019
This was referenced Apr 2, 2020
@PeterShaggyNoble PeterShaggyNoble self-assigned this Apr 8, 2020
This was referenced Apr 14, 2020
@PeterShaggyNoble PeterShaggyNoble mentioned this pull request Apr 20, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned Pull requests that have been abandoned by the contributor 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

5 participants