-
Notifications
You must be signed in to change notification settings - Fork 346
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ import 'app.dart'; | |
| import 'app_bar.dart'; | ||
| import 'button.dart'; | ||
| import 'color.dart'; | ||
| import 'content.dart'; | ||
| import 'icons.dart'; | ||
| import 'inbox.dart'; | ||
| import 'inset_shadow.dart'; | ||
|
|
@@ -307,29 +308,104 @@ void _showMainMenu(BuildContext context, { | |
| builder: (BuildContext _) { | ||
| return PerAccountStoreWidget( | ||
| accountId: accountId, | ||
| child: SafeArea( | ||
| minimum: const EdgeInsets.only(bottom: 8), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.stretch, | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| Flexible(child: InsetShadowBox( | ||
| top: 8, bottom: 8, | ||
| color: designVariables.bgBotBar, | ||
| child: SingleChildScrollView( | ||
| padding: const EdgeInsets.symmetric(vertical: 8, horizontal: 8), | ||
| child: Column(children: menuItems)))), | ||
| const Padding( | ||
| padding: EdgeInsets.symmetric(horizontal: 16), | ||
| child: AnimatedScaleOnTap( | ||
| scaleEnd: 0.95, | ||
| duration: Duration(milliseconds: 100), | ||
| child: BottomSheetDismissButton( | ||
| style: BottomSheetDismissButtonStyle.close))), | ||
| ]))); | ||
| child: _MainMenu(menuItems: menuItems)); | ||
| }); | ||
| } | ||
|
|
||
| /// 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 commentThe 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) |
||
| class _MainMenu extends StatelessWidget { | ||
| const _MainMenu({ | ||
| required this.menuItems, | ||
| }); | ||
|
|
||
| final List<Widget> menuItems; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final designVariables = DesignVariables.of(context); | ||
|
|
||
| return SafeArea( | ||
| minimum: const EdgeInsets.only(bottom: 8), | ||
| child: Column( | ||
| crossAxisAlignment: CrossAxisAlignment.stretch, | ||
| mainAxisSize: MainAxisSize.min, | ||
| children: [ | ||
| _MainMenuHeader(), | ||
| Flexible(child: InsetShadowBox( | ||
| top: 8, bottom: 8, | ||
| color: designVariables.bgBotBar, | ||
| child: SingleChildScrollView( | ||
| padding: const EdgeInsets.symmetric(vertical: 8, horizontal: 8), | ||
| child: Column(children: menuItems)))), | ||
| const Padding( | ||
| padding: EdgeInsets.symmetric(horizontal: 16), | ||
| child: AnimatedScaleOnTap( | ||
| scaleEnd: 0.95, | ||
| duration: Duration(milliseconds: 100), | ||
| child: BottomSheetDismissButton( | ||
| style: BottomSheetDismissButtonStyle.close))), | ||
| ])); | ||
| } | ||
| } | ||
|
|
||
| class _MainMenuHeader extends StatelessWidget { | ||
| const _MainMenuHeader(); | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| final zulipLocalizations = ZulipLocalizations.of(context); | ||
| 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 commentThe 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 |
||
|
|
||
| return Padding( | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
| child: Row(spacing: 8, children: [ | ||
| AvatarShape( | ||
| size: 28, | ||
| borderRadius: 4, | ||
| child: realmIconUrl != null | ||
| ? RealmContentNetworkImage(realmIconUrl) | ||
| : const SizedBox.shrink()), | ||
|
Comment on lines
+375
to
+376
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| Expanded(child: Text(store.realmName, | ||
| overflow: TextOverflow.ellipsis, | ||
| style: TextStyle( | ||
| color: designVariables.title, | ||
| fontSize: 20, | ||
| height: 24 / 20, | ||
| ).merge(weightVariableTextStyle(context, wght: 600)))), | ||
| ]))), | ||
| AnimatedScaleOnTap( | ||
| duration: const Duration(milliseconds: 100), | ||
| scaleEnd: 0.95, | ||
| child: TextButton( | ||
| onPressed: () { | ||
| Navigator.pop(context); // Close the main menu. | ||
| Navigator.of(context).push( | ||
| 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 commentThe reason will be displayed to describe this comment to others. Learn more. (Same comment about |
||
| foregroundColor: designVariables.icon, | ||
| 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 commentThe 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. |
||
| tapTargetSize: MaterialTapTargetSize.shrinkWrap), | ||
| child: Text(zulipLocalizations.organizationsButton, | ||
| style: const TextStyle(fontSize: 19, height: 26 / 19) | ||
| .merge(weightVariableTextStyle(context, wght: 600))))), | ||
| ])); | ||
| } | ||
| } | ||
|
|
||
| abstract class _MenuButton extends StatelessWidget { | ||
| const _MenuButton(); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import '../flutter_checks.dart'; | |
| import '../model/binding.dart'; | ||
| import '../model/store_checks.dart'; | ||
| import '../model/test_store.dart'; | ||
| import '../test_images.dart'; | ||
| import '../test_navigation.dart'; | ||
| import 'checks.dart'; | ||
| import 'test_app.dart'; | ||
|
|
@@ -225,6 +226,7 @@ void main () { | |
| } | ||
|
|
||
| testWidgets('navigation states reflect on navigation bar menu buttons', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
|
|
||
| await tapOpenMenuAndAwait(tester); | ||
|
|
@@ -238,9 +240,11 @@ void main () { | |
| await tapOpenMenuAndAwait(tester); | ||
| checkIconNotSelected(tester, inboxMenuIconFinder); | ||
| checkIconSelected(tester, channelsMenuIconFinder); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| testWidgets('navigation bar menu buttons control navigation states', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
|
|
||
| await tapOpenMenuAndAwait(tester); | ||
|
|
@@ -256,21 +260,27 @@ void main () { | |
| await tapOpenMenuAndAwait(tester); | ||
| checkIconNotSelected(tester, inboxMenuIconFinder); | ||
| checkIconSelected(tester, channelsMenuIconFinder); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| testWidgets('navigation bar menu buttons dismiss the menu', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
| await tapOpenMenuAndAwait(tester); | ||
| await tapButtonAndAwaitTransition(tester, channelsMenuIconFinder); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| testWidgets('close button dismisses the menu', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
| await tapOpenMenuAndAwait(tester); | ||
| await tapButtonAndAwaitTransition(tester, find.text('Close')); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| testWidgets('menu buttons dismiss the menu', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| addTearDown(testBinding.reset); | ||
| topRoute = null; | ||
| previousTopRoute = null; | ||
|
|
@@ -297,21 +307,27 @@ void main () { | |
| await tester.pump((topBeforePop as TransitionRoute).reverseTransitionDuration); | ||
|
|
||
| check(find.byType(BottomSheet)).findsNothing(); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| testWidgets('_MyProfileButton', (tester) async { | ||
| prepareBoringImageHttpClient(); | ||
| await prepare(tester); | ||
| await tapOpenMenuAndAwait(tester); | ||
| await tapButtonAndAwaitTransition(tester, find.text('My profile')); | ||
| check(find.byType(ProfilePage)).findsOne(); | ||
| check(find.text(eg.selfUser.fullName)).findsAny(); | ||
| debugNetworkImageHttpClientProvider = null; | ||
| }); | ||
|
|
||
| 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; | ||
| }); | ||
|
Comment on lines
323
to
331
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 :) |
||
| }); | ||
|
|
||
|
|
||
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.
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.