From 7a9cc831427d7779dc35ae0a508a6c786ac53a42 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Tue, 20 Feb 2024 14:29:54 -0800 Subject: [PATCH 1/7] ui: Stop setting deprecated ColorScheme.background MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We've started getting a deprecation warning from the analyzer: info • 'background' is deprecated and shouldn't be used. Use surface instead. This feature was deprecated after v3.18.0-0.1.pre • lib/widgets/app.dart:105:9 • deprecated_member_use The advice to use `surface` instead of `background` isn't right for us, for a few reasons: 1. ColorScheme.fromSeed always produces a ColorScheme with a non-null `background`; if you don't pass a `background`, it sets one for you, from MaterialDynamicColors. Some code downstream (ThemeData's constructor) uses ColorScheme.background preferentially over `surface`; I guess that code's behavior helps avoid breaking apps that haven't migrated away from `background`. Concretely, this means that if we followed the analyzer's recommendation to stop passing `background` to ColorScheme.fromSeed and pass `surface` instead, then the area under scrollable content like the message list would not be our chosen color, it would be a default color from MaterialDynamicColors. 2. Setting ColorScheme.surface would cause the chosen color to be used in at least one part of the UI where it wasn't used before: the app bar. We know our app bar isn't finalized, but I take this as a sign that `surface` isn't quite the right abstraction to fit the Figma's use of this color. Actually, `background` might already be the wrong abstraction: currently, it's not only used for the area under scrollable content like the message list, where the Figma wants it to be used. From reading code and checking empirically, it's also been getting picked up and used in two other places: the background of dialogs (as in our `showErrorDialog`), and our autocomplete popup. So, use something that seems like a better match for the Figma, and also works straightforwardly: ThemeData.scaffoldBackgroundColor. This does mean that dialogs and the autocomplete popup will be a slightly different shade of near-white than before (that default `background` courtesy of ColorScheme.fromSeed, the one from MaterialDynamicColors). That's fine. --- lib/widgets/app.dart | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 92ed8cb6a4..21fdef8773 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -99,11 +99,8 @@ class ZulipApp extends StatelessWidget { // https://m3.material.io/theme-builder#/custom colorScheme: ColorScheme.fromSeed( seedColor: kZulipBrandColor, - - // Used in the Figma for surfaces underneath scrollable content, e.g.: - // - background: const Color(0xfff6f6f6), ), + scaffoldBackgroundColor: 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. From 78c60eff2aa6775c7b77a56cd26169bb248dcd50 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 11:10:26 -0800 Subject: [PATCH 2/7] ui [nfc]: Use colorScheme.surfaceContainerHighest, not surfaceVariant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This straightforwardly resolves a warning from the analyzer without changing behavior: info • 'surfaceVariant' is deprecated and shouldn't be used. Use surfaceContainerHighest instead. This feature was deprecated after v3.18.0-0.1.pre • lib/widgets/compose_box.dart:815:26 • deprecated_member_use --- lib/widgets/compose_box.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/compose_box.dart b/lib/widgets/compose_box.dart index ea80368bd6..b68c7c0350 100644 --- a/lib/widgets/compose_box.dart +++ b/lib/widgets/compose_box.dart @@ -812,7 +812,7 @@ class _ComposeBoxLayout extends StatelessWidget { ); return Material( - color: colorScheme.surfaceVariant, + color: colorScheme.surfaceContainerHighest, child: SafeArea( minimum: const EdgeInsets.fromLTRB(8, 0, 8, 8), child: Padding( From 8e37ebb2ef1eacdf6ff828e28cc23d95fc65ebbf Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 11:50:53 -0800 Subject: [PATCH 3/7] ui: Prevent app-bar color change on scrolled-under state, again We successfully prevented this color change in 82eb41ee6, but an upstream Flutter change reintroduced a color change. Specifically, flutter/flutter@a2c7ed95d introduced ColorScheme.surfaceContainer and started using it for the scrolled-under state, in the case where the app bar hasn't been given a backgroundColor directly or by theme. --- lib/widgets/app.dart | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 21fdef8773..6275c0908c 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -84,10 +84,13 @@ class ZulipApp extends StatelessWidget { final theme = ThemeData( typography: zulipTypography(context), appBarTheme: const AppBarTheme( - // This prevents an elevation change in [AppBar]s so they stop turning - // darker if there is something scrolled underneath it. See docs: - // https://api.flutter.dev/flutter/material/AppBar/elevation.html + // Set these two fields to prevent a color change in [AppBar]s when + // there is something scrolled under it. If an app bar hasn't been + // given a backgroundColor directly or by theme, it uses + // ColorScheme.surfaceContainer for the scrolled-under state and + // ColorScheme.surface otherwise, and those are different colors. scrolledUnderElevation: 0, + backgroundColor: Color(0xfff5f5f5), ), // This applies Material 3's color system to produce a palette of // appropriately matching and contrasting colors for use in a UI. From be2f680fe617885045f0e8d732274459a823bd65 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 12:20:31 -0800 Subject: [PATCH 4/7] msglist: Follow Figma in app-bar background color in DM narrow Figma link: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=341-11956&mode=design&t=Nvsl6UdRBQvH3zXV-0 --- lib/widgets/message_list.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index f6203f7715..a5219bc0b3 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -65,7 +65,7 @@ class _MessageListPageState extends State { backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground ?? _kFallbackStreamColor; case DmNarrow(): - backgroundColor = _kFallbackStreamColor; + backgroundColor = _kDmRecipientHeaderColor; } return Scaffold( From 4590fd67131e4429bebb6a0314282c3cd15add63 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 12:33:00 -0800 Subject: [PATCH 5/7] msglist [nfc]: For all-messages narrow, let app-bar background inherit We recently set AppBarTheme.backgroundColor to be Color(0xfff5f5f5), which is the value of the constant we were using here. The constant wasn't perfectly appropriate here, because the color we want for the all-messages narrow has nothing to do with a "fallback stream" / unsubscribed stream color. --- lib/widgets/message_list.dart | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index a5219bc0b3..1359b74bb6 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -56,10 +56,10 @@ class _MessageListPageState extends State { Widget build(BuildContext context) { final store = PerAccountStoreWidget.of(context); - final Color backgroundColor; + final Color? backgroundColor; switch(widget.narrow) { case AllMessagesNarrow(): - backgroundColor = _kFallbackStreamColor; + backgroundColor = null; // i.e., inherit case StreamNarrow(:final streamId): case TopicNarrow(:final streamId): backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground From 5eb983fda953dfe23bbb1a2af1fc313637ab2669 Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 12:23:26 -0800 Subject: [PATCH 6/7] msglist [nfc]: s/_k{FallbackStream,UnsubscribedStreamRecipientHeader}Color/ This name is more parallel with _kDmRecipientHeaderColor. We could easily have dissolved this constant and just used AppBarTheme.backgroundColor, which is the same color (and which we started setting earlier in this series of commits). But it seems helpful to preserve the constant so it's easy to change. I don't think the Figma specifies a color for unsubscribed-stream recipient headers, and I think there's not much reason to prefer this color over some other color for that. --- lib/widgets/message_list.dart | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 1359b74bb6..564da85efb 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -47,7 +47,8 @@ class MessageListPage extends StatefulWidget { State createState() => _MessageListPageState(); } -const _kFallbackStreamColor = Color(0xfff5f5f5); +// TODO(design) this seems ad-hoc; is there a better color? +const _kUnsubscribedStreamRecipientHeaderColor = Color(0xfff5f5f5); class _MessageListPageState extends State { final GlobalKey _composeBoxKey = GlobalKey(); @@ -63,7 +64,7 @@ class _MessageListPageState extends State { case StreamNarrow(:final streamId): case TopicNarrow(:final streamId): backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground - ?? _kFallbackStreamColor; + ?? _kUnsubscribedStreamRecipientHeaderColor; case DmNarrow(): backgroundColor = _kDmRecipientHeaderColor; } @@ -640,7 +641,7 @@ class StreamMessageRecipientHeader extends StatelessWidget { : Colors.black; iconColor = swatch.iconOnBarBackground; } else { - backgroundColor = _kFallbackStreamColor; + backgroundColor = _kUnsubscribedStreamRecipientHeaderColor; contrastingColor = Colors.black; iconColor = Colors.black; } From 73e21fda4be7a6a9dd08b01d6c880a1dc124b1bd Mon Sep 17 00:00:00 2001 From: Chris Bobbe Date: Wed, 21 Feb 2024 12:54:34 -0800 Subject: [PATCH 7/7] ui: Follow Figma in adding app-bar bottom border The default app-bar color is now extremely similar to the scaffold background color -- Color(0xfff5f5f5) and Color(0xfff6f6f6), respectively -- and both follow the Figma. Because of their similarity, it seems helpful to delineate the app bar a bit better, and we can do so by adding this bottom border, which the Figma has: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?type=design&node-id=143-10772&mode=design&t=Nvsl6UdRBQvH3zXV-0 In the message list, the Figma isn't consistent on whether to omit the border when the app bar matches the color of all the recipient headers, like in a stream narrow. I think it's better to omit it in those cases, so I've done that here. --- lib/widgets/app.dart | 2 ++ lib/widgets/message_list.dart | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/lib/widgets/app.dart b/lib/widgets/app.dart index 6275c0908c..dc546707d6 100644 --- a/lib/widgets/app.dart +++ b/lib/widgets/app.dart @@ -91,6 +91,8 @@ class ZulipApp extends StatelessWidget { // ColorScheme.surface otherwise, and those are different colors. scrolledUnderElevation: 0, backgroundColor: Color(0xfff5f5f5), + + shape: Border(bottom: BorderSide(color: Color(0xffcccccc))), ), // This applies Material 3's color system to produce a palette of // appropriately matching and contrasting colors for use in a UI. diff --git a/lib/widgets/message_list.dart b/lib/widgets/message_list.dart index 564da85efb..93c4e9c1f5 100644 --- a/lib/widgets/message_list.dart +++ b/lib/widgets/message_list.dart @@ -58,20 +58,33 @@ class _MessageListPageState extends State { final store = PerAccountStoreWidget.of(context); final Color? backgroundColor; + bool removeAppBarBottomBorder = false; switch(widget.narrow) { case AllMessagesNarrow(): backgroundColor = null; // i.e., inherit + case StreamNarrow(:final streamId): case TopicNarrow(:final streamId): backgroundColor = store.subscriptions[streamId]?.colorSwatch().barBackground ?? _kUnsubscribedStreamRecipientHeaderColor; + // All recipient headers will match this color; remove distracting line + // (but are recipient headers even needed for topic narrows?) + removeAppBarBottomBorder = true; + case DmNarrow(): backgroundColor = _kDmRecipientHeaderColor; + // All recipient headers will match this color; remove distracting line + // (but are recipient headers even needed?) + removeAppBarBottomBorder = true; } return Scaffold( appBar: AppBar(title: MessageListAppBarTitle(narrow: widget.narrow), - backgroundColor: backgroundColor), + backgroundColor: backgroundColor, + shape: removeAppBarBottomBorder + ? const Border() + : null, // i.e., inherit + ), // TODO question for Vlad: for a stream view, should we set // [backgroundColor] based on stream color, as in this frame: // https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9684&mode=dev