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.

Sign up for GitHub

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

Jump to bottom

new icon: aerospike (original, original-wordmark) #2111

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

new icon: aerospike (original, original-wordmark) #2111

wants to merge 8 commits into from

Conversation

ah-aliakbarpour
Copy link

@ah-aliakbarpour ah-aliakbarpour commented Jan 14, 2024 •
edited
Loading

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 #2110

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

https://aerospike.com/

canaleal
canaleal reviewed Jan 15, 2024
View reviewed changes
Copy link
Member

@canaleal canaleal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for the contribution! Here are some changes that can be made to improve the PR:

Using Aliases

Because the aerospike-original.svg only has 1 path and a simple fill color #c21417, it can be used as a stand in (Alias) for the plain version.

More info about how to use aliases can be found here.

canaleal
canaleal requested changes Jan 15, 2024
View reviewed changes
Copy link
Member

@canaleal canaleal left a comment

Choose a reason for hiding this comment

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

Just a handful of small suggestions.

If you'd like to optimize the SVGs, please follow the steps in this tutorial

devicon.json Outdated Show resolved Hide resolved
devicon.json Outdated Show resolved Hide resolved
canaleal
canaleal requested changes Jan 16, 2024
View reviewed changes
Copy link
Member

@canaleal canaleal left a comment

Choose a reason for hiding this comment

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

Great work!

Because you used an alias, you can now delete the aerospike-plain.svg and takeout plain from the devicon.json file.

@ah-aliakbarpour
Copy link
Author

Thank you for reviewing!

I deleted the aerospike-plain.svg and pushed changes.

@Snailedlt Snailedlt changed the title new icon: aerospike (original, original-wordmark, plain) new icon: aerospike (original, original-wordmark) Jan 16, 2024
Snailedlt
Snailedlt requested changes Jan 16, 2024
View reviewed changes
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 for the most part!
The icons are excellent, so just a few minor details in the devicon.json left

devicon.json Outdated Show resolved Hide resolved
devicon.json Outdated Show resolved Hide resolved
Snailedlt
Snailedlt previously approved these changes Jan 16, 2024
View reviewed changes
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.

Excellent work!
Thank you for a great PR! 💯
image
image
image

@Snailedlt Snailedlt requested review from a team, ConX, weh and lunatic-fox and removed request for a team January 16, 2024 14:55
@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 Jan 16, 2024
weh
weh previously approved these changes Jan 18, 2024
View reviewed changes
Copy link
Contributor

@weh weh left a comment

Choose a reason for hiding this comment

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

Looking good 👍

@ah-aliakbarpour
Copy link
Author

@canaleal , What is the problem with requested changes?

canaleal
canaleal previously approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@canaleal canaleal left a comment

Choose a reason for hiding this comment

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

Great work. All is good.

@ah-aliakbarpour
Copy link
Author

Well, when will this PR be merged?

@weh
Copy link
Contributor

weh commented Feb 7, 2024

Well, when will this PR be merged?

the main focus was currently on the Release https://github.com/devicons/devicon/releases/tag/v2.16.0
so i guess this will happen soon, and is just a matter of time.
From your side, this MR is OK and ready, it has the needed reviews, so nothing to do left 👍

@ah-aliakbarpour
Copy link
Author

Thanks @weh.

@github-actions GitHub Actions
Copy link
Contributor

Hi!

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

devicon.json is not sorted correctly.
Please make sure that your icon is added in the `devicon.json` file at the correct alphabetic position
as seen here: https://github.com/devicons/devicon/wiki/Updating-%60devicon.json%60

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 😄

@canaleal canaleal dismissed stale reviews from Snailedlt and themself via bf23f8d February 17, 2024 18:08
@github-actions GitHub Actions
Copy link
Contributor

Hi!

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

devicon.json is not sorted correctly.
Please make sure that your icon is added in the `devicon.json` file at the correct alphabetic position
as seen here: https://github.com/devicons/devicon/wiki/Updating-%60devicon.json%60

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 😄

weh
weh approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@weh weh left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weh weh weh approved these changes

@canaleal canaleal canaleal approved these changes

@Snailedlt Snailedlt Snailedlt left review comments

@ConX ConX Awaiting requested review from ConX ConX was automatically assigned from devicons/devicon__reviewers

@lunatic-fox lunatic-fox Awaiting requested review from lunatic-fox lunatic-fox was automatically assigned from devicons/devicon__reviewers

Assignees
No one assigned
Labels
feature:icon Use this label for pull requests when a new icon is ready to be added to the collection
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ah-aliakbarpour @weh @Snailedlt @canaleal