From b3551e6c7a6e132e09755dd712614b6c15f9b8af Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Sun, 12 Nov 2023 22:20:16 -0500 Subject: [PATCH 1/4] recent_dm_conversations: Make rows white, like in Figma I missed this in #249 and made the rows transparent, oops. So they were taking on the scaffold background color, 0xfffafafa a.k.a. ThemeData.canvasColor. (If we were using Material 3, the scaffold background color would be 0xfffefbff a.k.a. ThemeData.colorScheme.background. Neither color is correct for what the Figma actually specifies for the surface underneath this screen's scrollable content. That's 0xfff6f6f6, and we'll start using it soon.) The ink effect on touch was being enabled by the scaffold's underlying Material. To preserve the ink effect, use a Material here instead of e.g. a ColoredBox. See the design in Figma: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=341%3A12378&mode=dev --- lib/widgets/recent_dm_conversations.dart | 54 ++++++++++++------------ 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/lib/widgets/recent_dm_conversations.dart b/lib/widgets/recent_dm_conversations.dart index f5932b2cb9..0122deb999 100644 --- a/lib/widgets/recent_dm_conversations.dart +++ b/lib/widgets/recent_dm_conversations.dart @@ -107,34 +107,36 @@ class RecentDmConversationsItem extends StatelessWidget { child: Icon(ZulipIcons.group_dm, color: Colors.black.withOpacity(0.5)))); } - return InkWell( - onTap: () { - Navigator.push(context, - MessageListPage.buildRoute(context: context, narrow: narrow)); - }, - child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 48), - child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ - Padding(padding: const EdgeInsetsDirectional.fromSTEB(12, 8, 0, 8), - child: AvatarShape(size: 32, borderRadius: 3, child: avatar)), - const SizedBox(width: 8), - Expanded(child: Padding( - padding: const EdgeInsets.symmetric(vertical: 4), - child: Text( - style: const TextStyle( - fontFamily: 'Source Sans 3', - fontSize: 17, - height: (20 / 17), - color: Color(0xFF222222), - ).merge(weightVariableTextStyle(context)), - maxLines: 2, - overflow: TextOverflow.ellipsis, - title))), - const SizedBox(width: 12), - unreadCount > 0 - ? Padding(padding: const EdgeInsetsDirectional.only(end: 16), + return Material( + color: Colors.white, + child: InkWell( + onTap: () { + Navigator.push(context, + MessageListPage.buildRoute(context: context, narrow: narrow)); + }, + child: ConstrainedBox(constraints: const BoxConstraints(minHeight: 48), + child: Row(crossAxisAlignment: CrossAxisAlignment.center, children: [ + Padding(padding: const EdgeInsetsDirectional.fromSTEB(12, 8, 0, 8), + child: AvatarShape(size: 32, borderRadius: 3, child: avatar)), + const SizedBox(width: 8), + Expanded(child: Padding( + padding: const EdgeInsets.symmetric(vertical: 4), + child: Text( + style: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 17, + height: (20 / 17), + color: Color(0xFF222222), + ).merge(weightVariableTextStyle(context)), + maxLines: 2, + overflow: TextOverflow.ellipsis, + title))), + const SizedBox(width: 12), + unreadCount > 0 + ? Padding(padding: const EdgeInsetsDirectional.only(end: 16), child: UnreadCountBadge(baseStreamColor: null, count: unreadCount)) : const SizedBox(), - ]))); + ])))); } } From c7be9c34de20f08f95aff40ccb741f193fbbaaa4 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 15 Nov 2023 11:08:06 -0500 Subject: [PATCH 2/4] content: Follow web app in using Source Sans 3, size 14, height 17 Values obtained by looking at the "Computed" tab in the Chrome DevTools element inspector, on CZO at 8.0-dev-2850-gef1c1ce05f. Technically the web app has a computed `line-height` of 16.996px, but that seems like a rounding error. One reason for doing this now is to opt out of a default that would take effect with the upcoming migration to Material 3. The text would otherwise get a line height of 1.43 times the font size, and we want it to be denser than that. We briefly explored how to preserve the line height exactly, across the M3 migration, but the solutions we found seemed more awkward than just taking care of this now: https://github.com/zulip/zulip-flutter/pull/380#issuecomment-1811819500 Fixes-partly: #157 Fixes-partly: #294 --- lib/widgets/content.dart | 9 ++++++++- test/widgets/content_test.dart | 2 +- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/widgets/content.dart b/lib/widgets/content.dart index 0da97d6aeb..3bdf524404 100644 --- a/lib/widgets/content.dart +++ b/lib/widgets/content.dart @@ -104,7 +104,14 @@ class Paragraph extends StatelessWidget { // The paragraph has vertical CSS margins, but those have no effect. if (node.nodes.isEmpty) return const SizedBox(); - final text = _buildBlockInlineContainer(node: node, style: null); + final text = _buildBlockInlineContainer( + node: node, + style: const TextStyle( + fontFamily: 'Source Sans 3', + fontSize: 14, + height: (17 / 14), + ).merge(weightVariableTextStyle(context)), + ); // If the paragraph didn't actually have a `p` element in the HTML, // then apply no margins. (For example, these are seen in list items.) diff --git a/test/widgets/content_test.dart b/test/widgets/content_test.dart index dac9bf0616..f97f348fa6 100644 --- a/test/widgets/content_test.dart +++ b/test/widgets/content_test.dart @@ -65,7 +65,7 @@ void main() { group('LinkNode interactions', () { // The Flutter test font uses square glyphs, so width equals height: // https://github.com/flutter/flutter/wiki/Flutter-Test-Fonts - const fontSize = 48.0; + const fontSize = 14.0; Future prepareContent(WidgetTester tester, String html) async { await testBinding.globalStore.add(eg.selfAccount, eg.initialSnapshot()); From 2f01c51fb423a16f48609139202f305128dcc08e Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Sun, 12 Nov 2023 23:04:30 -0500 Subject: [PATCH 3/4] ui: Migrate to Material Design 3 This completes the checklist at #225. I tested the screen-reader interface change with VoiceOver on my iPhone. Fixes: #225 --- lib/widgets/app.dart | 1 - lib/widgets/lightbox.dart | 3 +++ lib/widgets/login.dart | 20 +++++++------------- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 771fd4f53c..fd4c5fe816 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -92,7 +92,6 @@ class ZulipApp extends StatelessWidget { if (Theme.of(context).platform == TargetPlatform.iOS) '.AppleSystemUIFont' else 'sans-serif', 'Noto Color Emoji', ], - useMaterial3: false, // TODO(#225) fix things and switch to true // This applies Material 3's color system to produce a palette of // appropriately matching and contrasting colors for use in a UI. // The Zulip brand color is a starting point, but doesn't end up as diff --git a/lib/widgets/lightbox.dart b/lib/widgets/lightbox.dart index 4f47e9e919..7c3b2a4146 100644 --- a/lib/widgets/lightbox.dart +++ b/lib/widgets/lightbox.dart @@ -133,6 +133,7 @@ class _LightboxPageState extends State<_LightboxPage> { final appBarBackgroundColor = Colors.grey.shade900.withOpacity(0.87); const appBarForegroundColor = Colors.white; + const appBarElevation = 0.0; PreferredSizeWidget? appBar; if (_headerFooterVisible) { @@ -146,6 +147,7 @@ class _LightboxPageState extends State<_LightboxPage> { centerTitle: false, foregroundColor: appBarForegroundColor, backgroundColor: appBarBackgroundColor, + elevation: appBarElevation, // TODO(#41): Show message author's avatar title: RichText( @@ -167,6 +169,7 @@ class _LightboxPageState extends State<_LightboxPage> { if (_headerFooterVisible) { bottomAppBar = BottomAppBar( color: appBarBackgroundColor, + elevation: appBarElevation, child: Row(children: [ _CopyLinkButton(url: widget.src), // TODO(#43): Share image diff --git a/lib/widgets/login.dart b/lib/widgets/login.dart index affe0d6433..ed2e5a1f0e 100644 --- a/lib/widgets/login.dart +++ b/lib/widgets/login.dart @@ -392,19 +392,13 @@ class _PasswordLoginPageState extends State { decoration: InputDecoration( labelText: zulipLocalizations.loginPasswordLabel, helperText: kLayoutPinningHelperText, - // TODO(material-3): Simplify away `Semantics` by using IconButton's - // M3-only params `isSelected` / `selectedIcon`, after fixing - // https://github.com/flutter/flutter/issues/127145 . (Also, the - // `Semantics` seen here would misbehave in M3 for reasons - // involving a `Semantics` with `container: true` in an underlying - // [ButtonStyleButton].) - suffixIcon: Semantics(toggled: _obscurePassword, - child: IconButton( - tooltip: zulipLocalizations.loginHidePassword, - onPressed: _handlePasswordVisibilityPress, - icon: _obscurePassword - ? const Icon(Icons.visibility_off) - : const Icon(Icons.visibility))))); + suffixIcon: IconButton( + tooltip: zulipLocalizations.loginHidePassword, + onPressed: _handlePasswordVisibilityPress, + icon: const Icon(Icons.visibility), + isSelected: _obscurePassword, + selectedIcon: const Icon(Icons.visibility_off), + ))); return Scaffold( appBar: AppBar(title: Text(zulipLocalizations.loginPageTitle), From 21dbae1201fff3ac25e42e58e8d0d3c51d32c0b8 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Sun, 12 Nov 2023 23:30:29 -0500 Subject: [PATCH 4/4] ui: Use 0xfff6f6f6 for ColorScheme.background, following Figma From logging, this would otherwise be 0xfffefbff. See the Flutter doc on ColorScheme.background: https://api.flutter.dev/flutter/material/ColorScheme/background.html It simply says: > A color that typically appears behind scrollable content. This change is definitely the boldest we've been in perceiving a pattern in the Figma spec and organizing our code to fit it. We should cheerfully do something else if it turns out to make it harder instead of easier to follow the spec. Prompted by noticing that the surface underneath the scrollable content in RecentDmConversationsPage wasn't colored according to the Figma. And that the Figma's design for the Inbox page (#117) had the same color for the surface underneath *its* scrollable content. This change fixes the RecentDmConversationsPage and makes it easy and natural for the Inbox page to get the right color too. --- lib/widgets/app.dart | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index fd4c5fe816..a7fe91bee9 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -100,7 +100,13 @@ class ZulipApp extends StatelessWidget { // https://api.flutter.dev/flutter/material/ColorScheme/ColorScheme.fromSeed.html // Or try this tool to see the whole palette: // https://m3.material.io/theme-builder#/custom - colorScheme: ColorScheme.fromSeed(seedColor: kZulipBrandColor), + colorScheme: ColorScheme.fromSeed( + seedColor: kZulipBrandColor, + + // Used in the Figma for surfaces underneath scrollable content, e.g.: + // + background: const Color(0xfff6f6f6), + ), // `preferBelow: false` seems like a better default for mobile; // the area below a long-press target seems more likely to be hidden by // a finger or thumb than the area above.