-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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: jetpackcompose (original, original-wordmark,plain,plain-wordmark,line,line-wordmark) #1839
new icon: jetpackcompose (original, original-wordmark,plain,plain-wordmark,line,line-wordmark) #1839
Conversation
- added tag android
Co-authored-by: David Leal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sameemul-haque, great work on this PR, and making icons for all the different versions!
Overall the PR is good, but I've suggested some changes for you.
Other than the suggested change, here are some more things that need to be fixed before this PR can be accepted:
line
andplain
versions should have unified paths. See SVG-Versions for more info- The icons need to be centered and have as little padding as possible. i.e. they should fill the whole viewbox. For more info see SVG-Standards
Co-authored-by: Jørgen Kalsnes Hagen <[email protected]>
Hello @Snailedlt, I have updated the icons a863139. Could you please review them when you have some free time? Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent!
Imo the plain version still look a bit off though. Again, a preference thong so feel to disagree with me.
What I mean is that it looks like a slightly different logo than the line and original versions since the lines are not the same thickness, and don't have the same positions. Ideally the see-through lines on the plain version should be as thin as the lines on the line version. So you can almost think of it as an inverted line version if that makes sense.
I tried to point at the most obvious difference here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, this PR turned out really good in the end! 🥇
Thank you for your awesome contribution! Hope to see you contribute again! 🙇♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some small changes in all files
- removing the width and height attributes
- adding a newline at end of file
Co-authored-by: Patrik Wehrli <[email protected]>
Co-authored-by: Patrik Wehrli <[email protected]>
Co-authored-by: Patrik Wehrli <[email protected]>
Co-authored-by: Patrik Wehrli <[email protected]>
Co-authored-by: Patrik Wehrli <[email protected]>
Co-authored-by: Patrik Wehrli <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work on this PR!
Thanks again!
Double check these details before you open a PR
develop
branch.devicon.json
file as seen hereThis PR closes #1067
Link to prove your SVG is correct and up-to-date.