-
Notifications
You must be signed in to change notification settings - Fork 347
Show current organization's name and logo atop main menu #1937
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
base: main
Are you sure you want to change the base?
Conversation
|
Hm, looking at this now, I don't like the "Organizations" button there. It's not immediately apparent what organizations it's referring to/why. Where does it go? The account switcher? |
Correct in the current revision it goes to the account switcher button, also the Figma design doesn't have a separate "Switch account" button in the main menu. |
|
Can you just click on the org name itself to switch accounts? |
|
(If we're moving the account switching action to this new top line, we'll want to remove the "switch account" option in the same PR.) |
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! Comments below, and see also Alya's feedback (mentioning it so it doesn't get lost in the thread).
| String? get realmName => account.realmName; | ||
|
|
||
| Uri? get realmIcon => account.realmIcon; | ||
| // The `account` is populated with the `realmName` before | ||
| // PerAccountStore is created, so this should never be null. | ||
| // See `UpdateMachine.load`. | ||
| String get realmName => account.realmName!; | ||
|
|
||
| // The `account` is populated with the `realmIcon` before | ||
| // PerAccountStore is created, so this should never be null. | ||
| // See `UpdateMachine.load`. | ||
| Uri get realmIcon => account.realmIcon!; |
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.
store: Make realmName and realmIcon getters non-null
Commit-message nit: "non-nullable", right? Also the commit message repeats reasoning that's already clear from added code comments, so we can remove it from the commit message.
| testWidgets('_AboutZulipButton', (tester) async { | ||
| tester.view.physicalSize = Size(1080, 1920); | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
| await tapOpenMenuAndAwait(tester); | ||
| await tapButtonAndAwaitTransition(tester, find.byIcon(ZulipIcons.info)); | ||
| check(find.byType(AboutZulipPage)).findsOne(); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); |
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.
Rather than thinking through what's accomplished with the numbers 1080 and 1920 and whether they're correct, I'd rather just scroll until the button is in view:
testWidgets('_AboutZulipButton', (tester) async {
prepareBoringImageHttpClient();
await prepare(tester);
await tapOpenMenuAndAwait(tester);
await tester.ensureVisible(find.byIcon(ZulipIcons.info));
await tapButtonAndAwaitTransition(tester, find.byIcon(ZulipIcons.info));
check(find.byType(AboutZulipPage)).findsOne();
debugNetworkImageHttpClientProvider = null;
});That's simpler and more transparent, and also adds some coverage of the scrolling logic; it seems like a win-win :)
lib/widgets/home.dart
Outdated
| final designVariables = DesignVariables.of(context); | ||
| final store = PerAccountStoreWidget.of(context); | ||
|
|
||
| final realmIconUrl = store.tryResolveUrl(store.realmIcon.toString()); |
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.
Most of this line seems like code we'd rather not need to recite whenever we need the realm icon—should be possible to encapsulate it in PerAccountStore (maybe even Account?) where we have access to the realm URL.
lib/widgets/home.dart
Outdated
| padding: const EdgeInsets.only(top: 6), | ||
| child: Row(children: [ | ||
| Expanded(child: Padding( | ||
| padding: const EdgeInsets.fromLTRB(12, 6, 4, 6), |
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.
Use EdgeInsetsDirectional.fromSTEB here, for RTL support.
lib/widgets/home.dart
Outdated
| MaterialWidgetRoute(page: const ChooseAccountPage())); | ||
| }, | ||
| style: TextButton.styleFrom( | ||
| padding: const EdgeInsets.fromLTRB(8, 7, 14, 7), |
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.
(Same comment about EdgeInsetsDirectional.fromSTEB.)
lib/widgets/home.dart
Outdated
| ? RealmContentNetworkImage(realmIconUrl) | ||
| : const SizedBox.shrink()), |
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.
When the icon URL doesn't parse for whatever reason, and also when it does but the network request fails, how about we show an empty rounded square colored by DesignVariables.avatarPlaceholderBg? Maybe we could have a reusable RealmIconImage widget that takes care of that, much like AvatarImage takes care of choosing a placeholder (which it'll do in more cases with #1558).
lib/widgets/home.dart
Outdated
| /// The main-menu sheet. | ||
| /// | ||
| /// Figma link: | ||
| /// https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=11367-20647&t=lSnHudU6l7NWx0Fa-0 |
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.
This URL is taking me to the "Read receipts" sheet; do you reproduce? (Could be a Figma issue)
lib/widgets/home.dart
Outdated
| backgroundColor: Colors.transparent, | ||
| overlayColor: Colors.transparent, | ||
| splashFactory: NoSplash.splashFactory, | ||
| shape: RoundedRectangleBorder(borderRadius: BorderRadius.circular(10)), |
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.
This border radius shouldn't make a difference, with a transparent background/overlay/etc., right? I see it in the Figma, but it looks accidental there, too; I think we can leave it out without comment.
4e5d5f9 to
0d41609
Compare
|
Thanks for the reviews @chrisbobbe, @alya. |
This helper will be used soon to display organization logo in various places.
0d41609 to
1dc1fb7
Compare
1dc1fb7 to
c9c67d6
Compare
|
Seems reasonable to me, thanks! Open to feedback from @terpimost if he has another suggestion. |
Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=2940-48987&p=f&m=dev
Fixes: #1037