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

new icon: p5js (original) #1465

Merged
merged 13 commits into from
Nov 11, 2022
Merged

new icon: p5js (original) #1465

merged 13 commits into from
Nov 11, 2022

Conversation

yeyeto2788
Copy link
Contributor

@yeyeto2788 yeyeto2788 commented Oct 17, 2022

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened
  • PR name matches the format new icon: Icon name (versions separated by comma). More details here
  • PR's base is the develop branch.
  • Your icons are inside a folder as seen here
  • SVG matches the standards laid out here
  • A new object is added in the devicon.json file as seen here

This PR closes #1421

Link to prove your SVG is correct and up-to-date.

@Snailedlt Snailedlt self-requested a review October 19, 2022 13:22
@Snailedlt Snailedlt added the feature:icon Use this label for pull requests when a new icon is ready to be added to the collection label Oct 19, 2022
@Snailedlt Snailedlt self-assigned this Oct 19, 2022
@lunatic-fox
Copy link
Contributor

lunatic-fox commented Oct 22, 2022

Hello there!
@yeyeto2788 you're on the way, but there are few things that need to be fixed.

I think some linter is modifying the structure of devicon.json arrays. Perhaps, you can undo some steps until this file is in it's original structure.

Since this icon is a mono-color icon that contains the name of the technology, I think you can keep just the original version, rename it to original-wordmark and make an alias to plain-wordmark version.

And again the icon need to be in 128×128px size. If you need some help to do that on Inkscape, you can read the instructions bellow.

Instructions to resize to square

  • Select all. Ctrl+A
  • Group. Ctrl+G
  • Create a square with 128×128px of size.
  • Select all. Ctrl+A
  • Align the square and the icon to vertical center and horizontal center.
  • Resize the page to selection. Shift+Ctrl+R.
  • Delete the square.
  • Select the icon.
  • Ungroup. Shift+Ctrl+G
  • Save it and it's done!

About versions
https://github.com/devicons/devicon/wiki/SVG-Versions

About aliases
https://github.com/devicons/devicon/wiki/Example-of-Submitting-Icons-to-the-Repository

Wish for the best!

@yeyeto2788
Copy link
Contributor Author

Hey @lunatic-fox!

Thanks for pointing those instructions to me. I think I did everything to meet the requirements but if you still see something is missing or that I need to do something else just ping me.

Best.

@lunatic-fox
Copy link
Contributor

Hello again @yeyeto2788! Great work! 🤩

Just few issues:

@yeyeto2788
Copy link
Contributor Author

Hey there!

Changes applied, for this one it was a bit different as the Path > Union combination didn't work so I had to group them.

If something else is required, just let me know.

Best.

@lunatic-fox
Copy link
Contributor

Hello again @yeyeto2788!
Please take a look at my last comment on Jira Align PR, the same instructions can be applied here to this icon. 👍🏼

@yeyeto2788
Copy link
Contributor Author

I'm so sorry for all the inconveniences. I think now it should be fine.

Best.

@lunatic-fox
Copy link
Contributor

No worries! There is no inconvenience at all. 🙂
I didn't see before just two things: the altnames and tags properties are missing after name property.

Suggestion of altnames

"altnames": [
    "p5.js"
]

Suggestions of tags

"tags": [
    "javascript",
    "js",
    "library"
]

The icon is perfect now! 🎉

I remembered now that if you want you can also contact us on Discord server.

Copy link
Collaborator

@Snailedlt Snailedlt left a comment

Choose a reason for hiding this comment

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

Looks good, but some small changes are needed still before we can merge this :)

icons/p5js/p5js-original-wordmark.svg Outdated Show resolved Hide resolved
devicon.json Outdated Show resolved Hide resolved
@Snailedlt Snailedlt changed the title new icon: p5js (original,plain) new icon: p5js (original) Oct 29, 2022
yeyeto2788 and others added 3 commits October 31, 2022 07:33
Co-authored-by: Jørgen Kalsnes Hagen <43886029+Snailedlt@users.noreply.github.com>
@Snailedlt Snailedlt added the bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger label Oct 31, 2022
@github-actions
Copy link
Contributor

Hi!

I'm the check-bot and we have some issues with your PR:

SVG Error in 'p5js-original.svg':
- 'viewBox' is not '0 0 128 128' -> Set it or scale the file using https://www.iloveimg.com/resize-image/resize-svg.

Check our CONTRIBUTING guide for more details regarding these errors.

Please address these issues. When you update this PR, I will check your SVGs again.

Thanks for your help,
SVG-Checker Bot 😄

@github-actions
Copy link
Contributor

Hi there,

I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io.

Here are the SVGs as intepreted by Icomoon when we upload the files:
Imgur Images

Here are the zoomed-in screenshots of the added icons as SVGs:
Imgur Images

Here are the icons that will be generated by Icomoon:
Imgur Images

Here are the zoomed-in screenshots of the added icons as icons:
Imgur Images

Here are the colored versions:
Imgur Images

The maintainers will now check for:

  1. The number of Glyphs matches the number of SVGs that were selected.
  2. The icons (second group of pictures) look the same as the SVGs (first group of pictures).
  3. The icons are of high quality (legible, matches the official logo, etc.)

In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this doc for more details and fix the issues as instructed by Icomoon and update this PR once you are done.

Thank you for contributing to Devicon! I hope that your icons are accepted into the repository.

Note: If the images don't show up, it has been autodeleted by Imgur after 6 months due to our API choice.

Cheers,
Peek Bot 😊

@Snailedlt
Copy link
Collaborator

Hi!

I'm the check-bot and we have some issues with your PR:

SVG Error in 'p5js-original.svg':
- 'viewBox' is not '0 0 128 128' -> Set it or scale the file using https://www.iloveimg.com/resize-image/resize-svg.

Check our CONTRIBUTING guide for more details regarding these errors.

Please address these issues. When you update this PR, I will check your SVGs again.

Thanks for your help, SVG-Checker Bot 😄

@yeyeto2788 Please scale the image so that the viewbox is 0 0 128 128. You can use iloveimg to do it

@yeyeto2788
Copy link
Contributor Author

Quick note on the iloveimg tool. It does not work that great in Firefox and it does changes almost everything from the SVGOMG which I thought it was more or less the recommended tool.

@Snailedlt
Copy link
Collaborator

Quick note on the iloveimg tool. It does not work that great in Firefox and it does changes almost everything from the SVGOMG which I thought it was more or less the recommended tool.

Didn't know that it was bad on firefox, how so?
I see it changed the formatting and added width and height attributes, so if you run it through SVGOMG after resizing it, it should be better :)

@yeyeto2788
Copy link
Contributor Author

Yeap it is weird, seems like the preview does not work as expected and also sometimes if you want to set height and width one or the other gets erased.

image
image

Should I do that for my PRs? I mean, SVGOMG now that it is resized? (🙏🏽 for no)

@Snailedlt
Copy link
Collaborator

Snailedlt commented Oct 31, 2022

@yeyeto2788 odd indeed. I think it should work fine with chrome... at least it's always worked for me.

Also yes, please optimize it using SVGOMG :)

@yeyeto2788
Copy link
Contributor Author

@yeyeto2788 odd indeed. I think it should work fine with chrome... at least it's always worked for me.

Also yes, please optimize it using SVGOMG :)

Passed again through SVGOMG hoping it does not screwed the changes from iloveimg.

Best.

@Snailedlt Snailedlt added bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger and removed bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger labels Nov 3, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

Hi there,

I'm Devicons' Peek Bot and I just peeked at the icons that you wanted to add using icomoon.io.

Here are the SVGs as intepreted by Icomoon when we upload the files:
Imgur Images

Here are the zoomed-in screenshots of the added icons as SVGs:
Imgur Images

Here are the icons that will be generated by Icomoon:
Imgur Images

Here are the zoomed-in screenshots of the added icons as icons:
Imgur Images

Here are the colored versions:
Imgur Images

The maintainers will now check for:

  1. The number of Glyphs matches the number of SVGs that were selected.
  2. The icons (second group of pictures) look the same as the SVGs (first group of pictures).
  3. The icons are of high quality (legible, matches the official logo, etc.)

In case of font issues, it might be caused by Icomoon not accepting strokes in the SVGs. Check this doc for more details and fix the issues as instructed by Icomoon and update this PR once you are done.

Thank you for contributing to Devicon! I hope that your icons are accepted into the repository.

Note: If the images don't show up, it has been autodeleted by Imgur after 6 months due to our API choice.

Cheers,
Peek Bot 😊

Snailedlt
Snailedlt previously approved these changes Nov 3, 2022
Copy link
Collaborator

@Snailedlt Snailedlt left a comment

Choose a reason for hiding this comment

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

LGTM! ✔️
Well done, and thank you for your contribution! 🚀

@Snailedlt Snailedlt removed their assignment Nov 3, 2022
devicon.json Outdated Show resolved Hide resolved
Co-authored-by: Josélio Júnior <76992016+lunatic-fox@users.noreply.github.com>
lunatic-fox
lunatic-fox previously approved these changes Nov 9, 2022
Copy link
Contributor

@lunatic-fox lunatic-fox left a comment

Choose a reason for hiding this comment

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

Approved! ✔
Great contribution! 🚀

lunatic-fox
lunatic-fox previously approved these changes Nov 9, 2022
Copy link
Contributor

@lunatic-fox lunatic-fox left a comment

Choose a reason for hiding this comment

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

Merge conflicts resolved!

@lunatic-fox lunatic-fox requested a review from Snailedlt November 9, 2022 17:41
@lunatic-fox
Copy link
Contributor

Entry moved to its alphabetical order in devicon.json

@Snailedlt Snailedlt merged commit 6c377e7 into devicons:develop Nov 11, 2022
@yeyeto2788 yeyeto2788 deleted the p5js branch March 7, 2023 09:03
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
GCHQDeveloper926 pushed a commit to GCHQDeveloper926/devicon that referenced this pull request Dec 20, 2024
* Add icon for p5js

* Fix minor issues on the commit.

* Fix color for the icon and unite paths

* Fix paths and ungroup objects.

* Add altnames and tags to icon

* Update devicon.json

Co-authored-by: Jørgen Kalsnes Hagen <43886029+Snailedlt@users.noreply.github.com>

* Rename icon

* resize icon

* Pass icons through SVGOMG

* Update devicon.json

Co-authored-by: Josélio Júnior <76992016+lunatic-fox@users.noreply.github.com>

* Move entry to its alphabetical order

Co-authored-by: Jørgen Kalsnes Hagen <43886029+Snailedlt@users.noreply.github.com>
Co-authored-by: Josélio Júnior <76992016+lunatic-fox@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:peek Use this label to trigger peek-bot. Remove and re-add the label to re-trigger feature:icon Use this label for pull requests when a new icon is ready to be added to the collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants