msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

chrisbobbe · 2024-05-18T00:25:51Z

Like #657, but this is for the message list only (not content), and it completes my sweep through the message list. Screenshots coming soon.

chrisbobbe · 2024-05-18T01:01:06Z

Before	After

I don't have screenshots for the unsubscribed-stream icon color change, because (as discussed in the office just now) it doesn't seem possible to get to a message list that shows that icon.

chrisbobbe · 2024-05-20T00:42:00Z

Just pushed a small change (making the icon in the DM recipient header be the same color as the text) and updated the screenshots above, to show it.

gnprice

Thanks for cleaning these up, and the sleuthing on those date colors! Small comments below; otherwise looks good.

gnprice · 2024-05-20T18:55:18Z

lib/widgets/message_list.dart

-        color: color,
+        color: const HSLColor.fromAHSL(0.75, 0, 0, 0.15).toColor(),


I wonder if some confusion about contrastingColor (see a recent commit where we removed a variable with that name) might have led to the old color being used for stream headers, and perhaps the DM-header change was just done to align with stream headers.

Yeah. Glad to have this cleaned up! And good to have it consolidated here on DateText, too.

gnprice · 2024-05-20T18:58:40Z

lib/widgets/message_list.dart

+  // TODO(design) Do we want to match web more closely?
+  //   - width of 1 logical pixel instead of 1 physical pixel
+  //   - opacity 0.15


No, I'm pretty sure we want one physical pixel for a divider like this — one logical pixel is likely to look blurry.

In #469 (comment) I put both of these under "Other adjustments from web", not under "Design questions". So I don't think there's a TODO(design) to add here.

I see; yeah, that makes sense.

gnprice · 2024-05-20T19:06:00Z

lib/widgets/message_list.dart

@@ -892,6 +889,7 @@ class MessageWithPossibleSender extends StatelessWidget {

  final MessageListMessageItem item;

+  // TODO(#95) unchanged in dark theme?


Let's squash these three:

14c3f52 msglist [nfc]: Comment on _starColor about dark theme
a0ebda6 msglist [nfc]: Comment on _kMessageTimestampColor about dark theme
61be55b msglist [nfc]: Comment with TODO(#95) on mark-as-read button

oh and this previous commit:
53440c7 msglist [nfc]: Add TODO on date-separator line about dark theme

They're one line each, all adding comments on the same theme, and none of them call for significant discussion in the commit message.

gnprice · 2024-05-20T19:08:46Z

lib/widgets/message_list.dart

@@ -649,27 +657,15 @@ class StreamMessageRecipientHeader extends StatelessWidget {

    final subscription = store.subscriptions[message.streamId];
    final Color backgroundColor;
-    final Color contrastingColor;


Let's squash these two commits together:
71b4135 msglist [nfc]: Simplify a variable into a const
91ceae8 msglist [nfc]: Inline a variable

The squashed diff is still small and easy to follow, and I think makes the direction the refactor is going clearer than the first commit does alone.

When the background of stream recipient bars was colored by `subscription.color` itself, instead of a much-lightened transformation of that, it was presumably possible that ThemeData.estimateBrightnessForColor would give Brightness.dark, making `contrastingColor` be white. But when we started using `subscription.colorSwatch().barBackground`, in e97e6fd, the darkest possible color became #c8c8c8 (a light gray) -- that's from a base color of true black -- and for that color, ThemeData.estimateBrightnessForColor gives Brightness.light, making `contrastingColor` be black. Lighter base colors than true black will make even lighter `barBackground`s.

In web this is known as --color-text-message-header, and here is a Figma node: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538-20849&t=O4Yz8kKlRrXFY8Te-0 This is actually the same color that web uses -- and we use -- for message content. For that usage, web calls it --color-text-message-default. In fact, thanks to the DefaultTextStyle that wraps the whole message list (for the sake of coloring message content), DM recipient header text already had this color. So one option would have been to just remove the black color on stream recipient header text, to allow it to be colored via the same inheritance. But I didn't want to do that. Web has a reason for using separate variables: in dark theme, recipient header text is colored slightly different from message content. So, we'll keep these color definitions separate too, to make it easy to follow web in dark theme.

The Figma and the web app don't give something to follow here. But it seems probably appropriate to match the icon color to the text.

We're about to make this color follow web, and web's color doesn't differ between stream and DM recipient headers either.

The old color, which we started using for stream headers in e97e6fd and DM headers in 81f7038, doesn't match the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538-20849&t=ubXJTWC5kuz5awbN-0 #666666 or the web app: --color-date, which is hsl(0deg 0% 15% / 75%) I wonder if some confusion about contrastingColor (see a recent commit where we removed a variable with that name) might have led to the old color being used for stream headers, and perhaps the DM-header change was just done to align with stream headers. Anyway, might as well use the web app's color, which we were doing before e97e6fd. We can get a dark-theme variant from the web app too, which is nice. The Figma doesn't yet offer a dark-theme variant.

I removed the TODO(design), because now this element's color matches the corresponding element in the web app, and that seems like a fine way to settle the question. I didn't find a date-separator example in the Figma (and Greg didn't either, when implementing them in zulip#469.)

…eader The old color (added in a56577a) doesn't match the Figma: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538-20849&t=sbABZjnTMDdEpaTX-0 #9A9A9A or the web app: --color-message-header-icon-non-interactive, which is hsl(0deg 0% 0% / 30%)) Matching the web app is convenient because we can get a dark-theme variant from the web app. The Figma doesn't yet offer a dark-theme variant.

These belong in this StreamMessageRecipientHeader group.

A counterpart to the StreamMessageRecipientHeader group.

…olor As it does in the Figma. (Before, the color was the Material constant kDefaultIconDarkColor, which is Color(0xDD000000).)

chrisbobbe · 2024-05-20T20:52:56Z

Thanks for the review! Revision pushed.

gnprice · 2024-05-20T21:34:11Z

Thanks! Looks good; merging.

I overlooked this in zulip#686 because this text's color wasn't set at this site explicitly, and my process for that PR was to search case-insensitively for the term [color]. Instead of being set locally, this text's color was being set by the DefaultTextStyle that wraps the whole message list for the sake of coloring message content. That color doesn't match web, it turns out. (Or the old Figma, which uses true black.) Web colors this with var(--color-text-sender-name), but that variable is undefined for light theme, so it falls back to --color-text-default which is this color. At this commit, if I change the message list's DefaultTextStyle to set a conspicuous color, like orange, I only see a response from message content, and not from sender names or anything else. That's good, but I wonder if there's a less error-prone solution for coloring just the message content, without regressing on performance. I'll explore that later in this series.

I overlooked this in zulip#686 because this text's color wasn't set at this site explicitly, and my process for that PR was to search case-insensitively for the term [color]. Instead of being set locally, this text's color was being set by the DefaultTextStyle that wraps the whole message list for the sake of coloring message content. That color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor() doesn't match web, it turns out. (Or the old Figma, which uses true black.) Web colors this with var(--color-text-sender-name), but that variable is undefined for light theme, so it falls back to --color-text-default which is this color. At this commit, if I change the message list's DefaultTextStyle to set a conspicuous color, like orange, I only see a response from message content, and not from sender names or anything else. That's good, but I wonder if there's a less error-prone solution for coloring just the message content, without regressing on performance. I'll explore that later in this series.

I overlooked this in zulip#686 because this text's color wasn't set at this site explicitly, and my process for that PR was to search case-insensitively for the term [color]. Instead of being set locally, this text's color was being set by the DefaultTextStyle that wraps the whole message list for the sake of coloring message content. That color: const HSLColor.fromAHSL(1, 0, 0, 0.15).toColor() doesn't match web, it turns out. (Or the old Figma, which uses true black.) Web colors this with var(--color-text-sender-name), but that variable is undefined for light theme, so it falls back to --color-text-default which is this color. At this commit, if I change the message list's DefaultTextStyle to set a conspicuous color, like orange, I only see a response from message content, and not from sender names or anything else. That's good, but it would be better to scope the DefaultTextStyle to just the message content, to avoid errors like this in the future. I'll do that later in this series of commits.

chrisbobbe added a-design Visual and UX design integration review Added by maintainers when PR may be ready for integration labels May 18, 2024

chrisbobbe requested a review from gnprice May 18, 2024 00:25

chrisbobbe force-pushed the pr-update-msglist-colors branch 2 times, most recently from 467b821 to 0aeca52 Compare May 20, 2024 00:40

gnprice reviewed May 20, 2024

View reviewed changes

chrisbobbe added 18 commits May 20, 2024 13:36

msglist [nfc]: s/backgroundColor/appBarBackgroundColor/ for explicitness

8f25a7c

msglist [nfc]: Comment on unread-marker color for dark theme

42c5a68

msglist [nfc]: Inline a variable

6abafdd

msglist [nfc]: Deduplicate recipient-header text style

822e6ae

msglist: In unsubscribed stream, make icon color match text color

662a710

The Figma and the web app don't give something to follow here. But it seems probably appropriate to match the icon color to the text.

msglist [nfc]: Inline a variable

20c08c8

msglist [nfc]: Remove RecipientHeaderDate unnecessary color param

4c88148

We're about to make this color follow web, and web's color doesn't differ between stream and DM recipient headers either.

msglist [nfc]: Remove DateText unnecessary color param

41acd77

msglist [nfc]: Update a stale comment about zulip/zulip code

c0a75d0

msglist [nfc]: Add some TODO(zulip#95)s

72f0fd7

msglist test: Move two stream-recipient-header tests into their group

a027921

These belong in this StreamMessageRecipientHeader group.

msglist test: Make new DmRecpientHeader group; move two tests into it

42db773

A counterpart to the StreamMessageRecipientHeader group.

msglist: Make DM recipient header icon (ZulipIcons.user) match text c…

1439b8d

…olor As it does in the Figma. (Before, the color was the Material constant kDefaultIconDarkColor, which is Color(0xDD000000).)

chrisbobbe force-pushed the pr-update-msglist-colors branch from 0aeca52 to 1439b8d Compare May 20, 2024 20:52

gnprice merged commit 1439b8d into zulip:main May 20, 2024
1 check passed

chrisbobbe deleted the pr-update-msglist-colors branch May 20, 2024 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

chrisbobbe commented May 18, 2024

chrisbobbe commented May 18, 2024 •

edited

Loading

chrisbobbe commented May 20, 2024

gnprice left a comment

gnprice May 20, 2024

gnprice May 20, 2024

chrisbobbe May 20, 2024

gnprice May 20, 2024

gnprice May 20, 2024

chrisbobbe commented May 20, 2024

gnprice commented May 20, 2024

		color: color,
		color: const HSLColor.fromAHSL(0.75, 0, 0, 0.15).toColor(),

		@@ -892,6 +889,7 @@ class MessageWithPossibleSender extends StatelessWidget {

		final MessageListMessageItem item;

		// TODO(#95) unchanged in dark theme?

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

msglist: Update more elements closer to new, 2023+ design, to prepare for light/dark #686

Conversation

chrisbobbe commented May 18, 2024

chrisbobbe commented May 18, 2024 • edited Loading

chrisbobbe commented May 20, 2024

gnprice left a comment

Choose a reason for hiding this comment

gnprice May 20, 2024

Choose a reason for hiding this comment

gnprice May 20, 2024

Choose a reason for hiding this comment

chrisbobbe May 20, 2024

Choose a reason for hiding this comment

gnprice May 20, 2024

Choose a reason for hiding this comment

gnprice May 20, 2024

Choose a reason for hiding this comment

chrisbobbe commented May 20, 2024

gnprice commented May 20, 2024

chrisbobbe commented May 18, 2024 •

edited

Loading