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 Toshiba icon #1774

Merged
merged 5 commits into from Oct 25, 2019
Merged

Added Toshiba icon #1774

merged 5 commits into from Oct 25, 2019

Conversation

kaleeaswari
Copy link
Contributor

@kaleeaswari kaleeaswari commented Oct 14, 2019

Issue: #1661

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 15, 2019
@phatbhoy67
Copy link
Contributor

Hi @kaleeaswari thanks for your work on this PR, can you let me know what source you used for the .svg?

Looking at the .svg you submitted you will need to make some changes.

Did you follow the contributing guide instructions?

The .svg should have a single path, without colour or other attributes and be sized at 24 x 24. It should also be optimised.

If you need any help or tips on how to do that let me know.

As far as the logo itself, you have used the full Toshiba wordmark. Perhaps it is a good idea to use just the "T" like the favicon on the Toshiba website, what do the @simple-icons/maintainers think?

Copy link
Contributor

@birjj birjj left a comment

Choose a reason for hiding this comment

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

@phatbhoy67 I think that in this case the brand themselves use their wordmark as their identifying mark - although they use the "T" as a favicon, this doesn't seem to be used elsewhere. All their products and websites use the full wordmark, so that's probably what we also want to use. Good call on bringing it up, though.

@kaleeaswari Could you please update the PR to meet our contribution guidelines (as linked by @phatbhoy67 above)?

@birjj birjj added awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed changes requested and removed awaiting reply Issues or pull requests awaiting reply from an individual before it may be addressed labels Oct 19, 2019
@kaleeaswari
Copy link
Contributor Author

Have committed new file created using Inkscape and optimized using SVGOMG.

@phatbhoy67
Copy link
Contributor

Have committed new file created using Inkscape and optimized using SVGOMG.

Hi @kaleeaswari thanks for making the changes, however there are a few changes you still need to make to your .svg file.

The easiest thing to do would be to open it up again in Inkscape and edit it.

Are you quite familiar with Inkscape? If not I can list some steps you need to take to achieve the result in detail, let me know.

In summary the steps are:

  • open file in Inkscape
  • select your "toshiba" object
  • object->ungroup
  • unset fill
  • center on page (horizontally and vertically)
  • open document properties and set width and height to 24px, ensure "scale" is set to "1"
  • set viewbox to 24px by 24px
  • select your "toshiba" object, lock the aspect ratio and set width to 24px
  • center again on page both horizontally and vertically

That should fix some of the issues.

Then you will need to optimise it and annotate it.

You can annotate it easily using a normal text editor.

Hope that helps, let me know if you need any further guidance 👍

@kaleeaswari
Copy link
Contributor Author

Thanks for the detailed steps phatbhoy67. Am new to inkscape, have followed the steps listed and committed back.

@phatbhoy67
Copy link
Contributor

Thanks for updating the file @kaleeaswari It looks like we are nearly there. The icon is still too big, I think you may have set the height to 24px rather than the width.

If you open the file back up in Inkscape, select the icon, make sure the aspect ratio is locked (click the padlock in between the W and H) and set the width to 24px.

Then center the icon both vertically and horizontally on the page.

Next, to ensure you have only one path, with the icon selected, go to the "Path" menu and click "union". Nothing should change visually when you do this.

Save the file and upload it to svgomg check it looks ok , if so, click the "markup" button. There should be only one <path .... /> which should start with d="M and end with a z".

If that is the case press the "download" button. Open that file in a text editor.

The properly annotated final file should look like this (I've removed the path data to shorten it in the example)

<svg role="img" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><title>Toshiba icon</title><path d="M.....z"/></svg>

To be clear that means you will have to manually delete the version="1.0" width="24" height="24" from the file. It's totally fine to do that.

That should be you good to go.

Good luck and let me know if you get stuck 👍

@phatbhoy67
Copy link
Contributor

phatbhoy67 commented Oct 24, 2019

Hey @kaleeaswari the .svg looks good, well done and thanks for your work making the requested changes 👍. The reason the checks are still failing is because you have used lowercase letters in the hex value in the _data/simple_icons.json file. If you change the ff0000 to FF0000 that will fix the issue and pass the check. 🤞 Can you also move the entry to be in alphabetical order, the Toshiba entry should be between the entries for tor.svg and trainerroad.svg. You can simple cut and paste it, be careful to include a comma after your entry though. Good luck!

@kaleeaswari
Copy link
Contributor Author

Thanks much @phatbhoy67 for helping out with detailed explanations.

@phatbhoy67
Copy link
Contributor

Hey @kaleeaswari thanks for your patience, persistence and effort on this PR, really appreciated the contribution 👍. I will ask @birjolaxew to review it, hopefully it will be merged in soon. 👏

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

Thanks @phatbhoy67.. Looking forward for merge.

@birjj
Copy link
Contributor

birjj commented Oct 25, 2019

👍 Looks good! Thanks a lot for all your work on updating this @kaleeaswari and @phatbhoy67!

@birjj birjj merged commit 92a6c18 into simple-icons:develop Oct 25, 2019
@phatbhoy67 phatbhoy67 mentioned this pull request Oct 26, 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