-
Notifications
You must be signed in to change notification settings - Fork 160
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
icons: Updated bot icon with single antenna version. #986
Conversation
Please write down specifically where you got the new icon from. The commit message is a good place for that, and the first place someone in the future will look when they want to know where it came from. |
pubspec.lock
Outdated
@@ -1060,10 +1060,10 @@ packages: | |||
dependency: "direct dev" |
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.
Looks like the update to this file is unintended.
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.
Yes, it was unintended. It was changed when I ran a flutter pub get
Yes, the icon was gotten from the |
Should I send an updated commit with a description of where I got the icon from or adding a comment here would be fine? @chrisbobbe |
Yes, please update the commit. As I said above:
|
Okay, understood, thank you. |
5998632
to
92c7591
Compare
I have updated the commit @chrisbobbe |
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.
Thanks! Comment below, and it looks like you still need to remove the accidental pubspec.lock
changes.
assets/icons/bot.svg
Outdated
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.
icons: Updated bot icon with single antenna version.
Icon gotten from the Github Zulip repository at https://github.com/zulip/zulip/blob/main/web/shared/icons/bot.svg
Notes on the commit message:
-
Thanks for including a URL for the SVG. It may become out of date, though, as the
main
branch advances in the zulip/zulip repository. So let's point to a specific commit. The commit that added the file would be a good one:- https://github.com/zulip/zulip/blob/main/web/shared/icons/bot.svg + https://github.com/zulip/zulip/blob/030f93595/web/shared/icons/bot.svg
-
In zulip-flutter the summary line is in the imperative and without a final period, so: "icons: Update bot icon with single antenna version"
-
Wrap the text of the commit-message body to 68 columns
-
Add a
Fixes: #978
line (in the commit and also the PR description)
For examples of merged commit messages, see the project's history.
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.
Okay, noted, thank you
92c7591
to
ec6cddc
Compare
Ah, I see you've also wrapped the URL onto two lines :) but we do like to keep URLs intact instead of breaking them. So that can be an exception to the wrap-to-68-columns rule. |
Yeah, I was actually confused about what to do with URLs, let me update that then. |
ec6cddc
to
ce1ecf9
Compare
Updated |
Ah, and one thing I forgot in my previous reviews: since this makes a visible change, please post before/after screenshots. The code and commit message look good to me though, and I'll move this along to Greg for his review (but please do post those screenshots). |
Oh, I see you did post screenshots, just on the issue thread instead of here: #978 (comment) In future, please post the screenshots on the PR thread. But I do think those screenshots look good. |
Icon gotten from the Github Zulip repository at https://github.com/zulip/zulip/blob/030f93595/web/shared/icons/bot.svg Fixes: zulip#978
Thanks @fombalang for taking care of this, and @chrisbobbe @PIG208 for the previous reviews! This looks good; merging. I fixed one nit in the commit message:
|
ce1ecf9
to
72b6657
Compare
This PR updates the previous bot icon, replacing it with a new single antenna bot icon.
No tests have been written or updated for this PR as the changes made were minimal and did not involve changing any dart code, just updating the SVG asset file and rebuilding the icon font.
Fixes: #978
Thank you.