Skip to content

Conversation

@Snailedlt
Copy link
Collaborator

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

This PR closes NONE

It is however related to #1327

Notes

@Snailedlt Snailedlt changed the base branch from master to develop July 28, 2022 19:41
@Snailedlt Snailedlt force-pushed the feature/add_alphabetical_ordering_of_devicon_json_to_pr_checklist branch from 18f017b to 660781a Compare July 28, 2022 19:41
@Snailedlt
Copy link
Collaborator Author

@BenSouchet does this look good in your opinion?

@kilianpaquier
Copy link
Contributor

Maybe the new_icon.md PR template should add the checklist item too ?

@BenSouchet
Copy link
Contributor

BenSouchet commented Jul 28, 2022

@Snailedlt Yep its looks good to me.

And I agree with @kilian-paquier this file:
https://github.com/devicons/devicon/blob/develop/.github/PULL_REQUEST_TEMPLATE/new_icon.md

could be also edited like:
from

A new object is added in the devicon.json file as seen here

to

A new object is added in the devicon.json file at the correct alphabetic position as seen here

@Snailedlt
Copy link
Collaborator Author

Maybe the new_icon.md PR template should add the checklist item too ?

I'm not sure exactly what you mean, could you elaborate? Perhaps an example of exactly what should be updated?

Panquesito7
Panquesito7 previously approved these changes Jul 28, 2022
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Looking great! Thank you. 🚀

I agree with what @kilian-paquier and @BenSouchet suggested.
We can implement the changes on this same PR and then merge them.

__
sema-logo  Summary: 🏆 This code is awesome  |  Tags: Efficient, Maintainable

@kilianpaquier
Copy link
Contributor

Maybe the new_icon.md PR template should add the checklist item too ?

I'm not sure exactly what you mean, could you elaborate? Perhaps an example of exactly what should be updated?

Hello, sorry, I meant updating this markdown PR template https://github.com/devicons/devicon/blob/master/.github/PULL_REQUEST_TEMPLATE/new_icon.md with a new check

@Snailedlt
Copy link
Collaborator Author

@kilian-paquier Ahh, yes I agree. Will add it there too!

@Snailedlt
Copy link
Collaborator Author

@Panquesito7 @BenSouchet @kilian-paquier
Implemented the suggested changes. Let me know if you find anything else

Panquesito7
Panquesito7 previously approved these changes Jul 28, 2022
Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

__
sema-logo  Summary: 🏆 This code is awesome  |  Tags: Efficient, Maintainable

@BenSouchet
Copy link
Contributor

@Snailedlt In the Files Changed section I don't see the modification of the new_icon.md file, is it normal ?

@Snailedlt
Copy link
Collaborator Author

@Snailedlt In the Files Changed section I don't see the modification of the new_icon.md file, is it normal ?

Yeah I noticed that too. Happened after the merging through the GitHub UI for some reason. I'll mark the PR as a draft until I fix it

@Snailedlt Snailedlt marked this pull request as draft August 1, 2022 13:22
@Snailedlt Snailedlt added the devops Devops/automation related enhancements label Oct 1, 2022
@Snailedlt Snailedlt force-pushed the feature/add_alphabetical_ordering_of_devicon_json_to_pr_checklist branch from 27bd23d to b0a387e Compare October 1, 2022 14:55
@Snailedlt Snailedlt marked this pull request as ready for review October 1, 2022 14:59
@Snailedlt Snailedlt requested a review from Panquesito7 October 1, 2022 14:59
@Snailedlt
Copy link
Collaborator Author

Ready for review again!
I rebased on develop, and fixed the link to the updating devicon.json wiki page now.

@kilian-paquier @Panquesito7 @BenSouchet
Please look through the changes again and review them, thanks! :)

@BenSouchet
Copy link
Contributor

Hi 👋, I checked, and on my side I approve the changes 👍

@kilianpaquier
Copy link
Contributor

Hello, fine on my side

@Snailedlt
Copy link
Collaborator Author

@Panquesito7 still waiting for a review here :)

@Snailedlt Snailedlt added the hacktoberfest-accepted Accepted to be counted towards Hacktoberfest label Oct 31, 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.

LGTM! ✔

@Snailedlt Snailedlt merged commit 300685a into devicons:develop Dec 11, 2022
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
GCHQDeveloper926 pushed a commit to GCHQDeveloper926/devicon that referenced this pull request Dec 20, 2024
…#1332)

* Add note to maintainers about checking devicon.json

related devicons#1327

* implement suggest changes from PR reviews

* fix updating devicon.json wiki link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops Devops/automation related enhancements enhancement hacktoberfest-accepted Accepted to be counted towards Hacktoberfest

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants