Icon for "switch account" in main menu #1338
Conversation
|
This has many changes unrelated to #1178; those need to be removed before we can review. |
|
The changes should only be to the following files right?:
|
|
Determining exactly which files should be changing is part of the work of resolving this issue (or any issue) 🙂. If you need help or have questions, the place to ask is in #mobile-dev-help on chat.zulip.org. In any event, the essential part is that you should reread your changes and understand why each change you're making needs to be there: |
|
Hey @gnprice can you check the pull request once? I hope I have made the necessary changes! if not ill work on it again. |
There was a problem hiding this comment.
Thanks. The commit message needs a "Fixes" line (see the project's commit history for examples), and please answer my question about the icon, below. When that's settled, please update the screenshots in your PR description, so we can see what the change will look like if merged.
|
@chrisbobbe done with the changes, could you review it please? |
There was a problem hiding this comment.
Thanks for the revision! Let's actually split this into two commits:
- One that adds the icon (for examples, see
git log assets/icons/using Greg's "secret" for reading Git history) - One that uses the icon where it's needed; this one should have the line
Fixes #1178.(note how that line is formed; we sometimes useFixes: #1178interchangeably).
|
@chrisbobbe I have made the necessary modifications, please check! |
|
The commit message has extra words in the "Fixes" line. See my two previous messages for how to form that line. |
|
Thanks for the revision! Marking for Greg's review. |
chrisbobbe
added
integration review
| @@ -27,116 +27,119 @@ abstract final class ZulipIcons { | |||
| /// The Zulip custom icon "arrow_down". | |||
| static const IconData arrow_down = IconData(0xf101, fontFamily: "Zulip Icons"); | |||
|
|
|||
| /// The Zulip custom icon "arrow_left_right". | |||
| static const IconData arrow_left_right = IconData(0xf102, fontFamily: "Zulip Icons"); | |||
There was a problem hiding this comment.
The first commit doesn't pass tools/check:
$ git checkout main~
$ tools/check
[…]
Running icons...
Error: there were icon updates:
M lib/widgets/icons.dart
Running android...
FAILED: icons
To rerun the suites that failed, run:
$ tools/check icons
It's important that each commit make sense on its own, including passing all tests. See our guide on coherent commits:
https://zulip.readthedocs.io/en/latest/contributing/commit-discipline.html
There was a problem hiding this comment.
This is the result that i get when I run tools/check on my branch:
All tests passed!
Running flutter_version...
Running build_runner...
Running l10n...
Running drift...
Running pigeon...
Running icons...
Running android...
Passed!
Though running this command makes changes to my pubspec.lock file. Should I commit that file as well?
There was a problem hiding this comment.
This is the result that i get when I run
tools/checkon my branch:
Right, it passes at the tip of the branch. In order to make each commit coherent, all tests need to pass at each individual commit along the way as well.
See the steps I wrote above, and the linked doc. If you have further questions, #git help or #mobile-dev-help would be good places to ask (you'll get faster responses than on GitHub).
There was a problem hiding this comment.
Though running this command makes changes to my
pubspec.lockfile. Should I commit that file as well?
Typically that happens when you're using a new version of Flutter than the one that pubspec.lock was last updated for. You'll see the same changes happen when you run the app, too 🙂.
And you'll see the same changes when running the app, or tools/check, on main without your PR. So they're independent of your other changes, and shouldn't be committed together.
If you'd like to upgrade the Flutter version in the repo, a PR for that would be welcome. See the instructions in our README.
lib/widgets/home.dart
Outdated
|
|
||
| @override | ||
| Widget buildLeading(BuildContext context) => const SizedBox.shrink(); | ||
|
|
There was a problem hiding this comment.
nit: don't separate @override from its target with a blank line — match the style of the existing code you see
There was a problem hiding this comment.
Fixed by removing the blank line!
| @override | ||
| Widget buildLeading(BuildContext context) => const SizedBox.shrink(); | ||
|
|
||
| IconData? get icon => ZulipIcons.arrow_left_right; |
There was a problem hiding this comment.
nit:
icons: Add switch account icon in main menu
The "icons" prefix isn't right for describing this commit — that's used for introducing icons to the tree, like your first commit. (For examples, see git log --oneline --stat --full-diff assets/icons/. Or from the other direction: git log --oneline --stat --grep 'icons:'.)
Instead, match the prefix used for other changes aimed at this file.
There was a problem hiding this comment.
Seeing changes in home.dart file using git log --oneline --stat --full-diff lib/widgets/home.dart, I think commit summary home: Add switch account icon in main menu would be appropriate!
The icon has been taken from figma as requested: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=6141-40422&t
fixes: #1178