ui: Stop using deprecated ColorScheme.background and ColorScheme.surfaceVariant
#527
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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
We successfully prevented this color change in 82eb41e, 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.
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.
…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.
gnprice
reviewed
Feb 21, 2024
Comment on lines
-68
to
+75
| backgroundColor = _kFallbackStreamColor; | ||
| backgroundColor = _kDmRecipientHeaderColor; |
There was a problem hiding this comment.
Ah good catch, thanks. I definitely see the difference in the DM narrow's app bar in the screenshots, and this looks better.
lib/widgets/message_list.dart
Outdated
Comment on lines
71
to
74
| removeAppBarBottomBorder = true; | ||
| case DmNarrow(): |
There was a problem hiding this comment.
nit: separate the cases (here and above) visually, now that they're longer:
Suggested change
| removeAppBarBottomBorder = true; | |
| case DmNarrow(): | |
| removeAppBarBottomBorder = true; | |
| case DmNarrow(): |
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.
chrisbobbe
force-pushed
the
pr-deprecated-color-scheme-fields
branch
from
ae0e2ce to
73e21fd
Compare
|
Thanks! Done. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After upgrading Flutter I get this feedback from the analyzer:
Discussion on CZO: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20some.20.60ColorScheme.60.20fields.20deprecated/near/1741410
This fixes those analyzer issues.
While investigating, I discovered that the same upstream change also did something else: it re-introduced a color change in our default app bar when content scrolls under it.
That's fixed straightforwardly by setting a constant background color ourselves, instead of deferring to Material's defaults. We choose a background color from the Figma, naturally…and since that color is very similar to the scaffold background color, also from the Figma, I go ahead and apply the Figma's bottom border to the app bar. There's some subtlety around removing that bottom border for some cases in the message list. Here are some screenshots of the relevant cases.
I also set the appropriate app-bar background for DM narrows, following the Figma.