Redesigned stream/topic recipient headers #416

sirpengi · 2023-11-24T18:42:54Z

Design from the following two sources:

This PR updates the stream/topic recipient headers to the new design and a PR to #373 will follow this up very shortly. The style of dates were updated, but the format of the text itself is still left for #411

This fixes #372 and fixes #220.
I made an attempt to tackle #282 using combinations of Row/Expanded/Flexible but wasn't able to succeed. I suspect we will need to implement some custom layout code to handle that.

sirpengi · 2023-11-24T18:47:21Z

@gnprice ready for review!

gnprice

Thanks @sirpengi! Generally this looks good. Comments below, mostly small.

gnprice · 2023-11-27T00:11:15Z

lib/widgets/message_list.dart

@@ -46,13 +47,30 @@ class MessageListPage extends StatefulWidget {
  State<MessageListPage> createState() => _MessageListPageState();
 }

+final _kFallbackStreamColorSwatch = StreamColorSwatch(0xfff6f6f6);


In the screenshot for "All messages", the app bar looks quite a bit darker than it does in the Figma design. Can you describe where this #f6f6f6 color came from?

Should it perhaps be Color(0xfff6f6f6) itself, rather than the StreamColorSwatch.barBackground that we get from treating that color as if it were a stream color?

I traced back my usage of #f6f6f6 from https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=132%3A9617&mode=dev but a lot of other screens show #f5f5f5 (https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=538%3A20850&mode=dev which I believe are more up to date) so I'll make that small tweak.

Although bigger picture is, yes, this should be the end color, and not the swatch that gets passed in! I'll update so we get the right fallback colors.

gnprice · 2023-11-27T00:13:33Z

lib/widgets/message_list.dart

    final message = this.message;
    return switch (message) {
-      StreamMessage() => StreamTopicRecipientHeader(message: message),
+      StreamMessage() => StreamMessageRecipientHeader(message: message, showStream: narrow is AllMessagesNarrow),


nit: break line

gnprice · 2023-11-27T00:13:41Z

lib/widgets/message_list.dart

-class StreamTopicRecipientHeader extends StatelessWidget {
-  const StreamTopicRecipientHeader({super.key, required this.message});
+class StreamMessageRecipientHeader extends StatelessWidget {
+  const StreamMessageRecipientHeader({super.key, required this.message, required this.showStream});


nit: break line

gnprice · 2023-11-27T00:16:52Z

lib/widgets/message_list.dart

+      height: (18 / 16),
+    ).merge(weightVariableTextStyle(context, wght: 600, wghtIfPlatformRequestsBold: 900));
+
+    final streamWidget = (showStream)


nit: redundant parens

Suggested change

final streamWidget = (showStream)

final streamWidget = showStream

gnprice · 2023-11-27T00:21:02Z

lib/widgets/message_list.dart

+                (stream != null) ? iconDataForStream(stream) : null)),
+            Padding(
+              padding: const EdgeInsets.symmetric(vertical: 11),
+              child: Text(stream?.name ?? message.displayRecipient, // TODO(log) if missing


This gets a bit crowded. Imagine in particular what it'll look like after we fulfill that TODO(log) — we'll then definitely want a bit of explicit statement-level control flow. But I think even at this point, forcing all of this inside an expression is already a bit crowded.

Instead of using the ?: operator, this can look like:

final Widget streamWidget; if (!showStream) { streamWidget = SizedBox(width: 16); } else { // … now there's room for local variables here, and conditionals, then … streamWidget = GestureDetector( // …

That way there can be a streamName local, just as there is in main.

gnprice · 2023-11-27T01:01:10Z

lib/widgets/message_list.dart

+              // Figma shows 5px spacing but 6px here matches better visually.
+              padding: const EdgeInsets.symmetric(horizontal: 6, vertical: 11),


Hmm interesting. What does it make a better match with — does it better match the result to what you see in Figma, or match this to another part of the design, or something else?

Adjusted the comment; it matches what we see in Figma more.

Curious. I wonder what the discrepancy is, then — in principle if we match the numbers in the Figma spec, we should get the same visual results as the Figma preview.

Given the Figma bug we just discovered about opacity, it wouldn't be shocking if this was a bug in Figma where its preview didn't interpret its spec correctly. But it could also be a bug in our translation of the spec.

On further examination here it looks like the icon is given a size of 18px but the icon itself has width/height of 16px here (there is 1px of space inside the icon!) and so there are 6 pixels of whitespace from the left border:

Another result of this examination is I adjusted the icon size to be 16px instead.

Hmm I see — and our hash_sign.svg has the tips of the strokes actually touching the edge of the canvas:

so it corresponds to that 16x16 area in the middle. That's the discrepancy, then — glad to have an explanation for it. @terpimost FYI.

gnprice · 2023-11-27T01:05:04Z

lib/widgets/message_list.dart

+          onTap: () => Navigator.push(context,
+            MessageListPage.buildRoute(context: context,
+              narrow: StreamNarrow(message.streamId))),
+          child: Row(children: [


This needs CrossAxisAlignment.baseline, just like the other Row below. Otherwise the vertical alignment gets messed up — it's most conspicuous if you crank up your text size in system settings:

gnprice · 2023-11-27T01:14:22Z

lib/widgets/message_list.dart

+                // A null [Icon.icon] makes a blank space.
+                (stream != null) ? iconDataForStream(stream) : null)),
+            Padding(
+              padding: const EdgeInsets.symmetric(vertical: 11),


What's the origin of the number 11 for vertical padding? I'm not finding that in the Figma design.

Also, it seems like it'd be simpler to apply the 11px vertical padding to the entire Row. Is there a reason I'm missing why that wouldn't have the right effect?

This is reverse engineered from a 40px height given the icon height (18p) for this element, and was also applied to the text widget that has 18px line height. The chevron has 16 height and thus given vertical: 12. I don't think it's clear that wrapping the whole row is an improvement as the chevron would need padding as well.

I'll take a closer look at the behavior of cranking up text size in the system settings. In your screenshot it seems that this doesn't affect icon sizes, in which case it might make sense to apply a uniform padding on text elements and vertically center the icons another way.

After our call today I've settled on applying a CrossAxisAlignment.center to all the row elements and applying a vertical: 11 padding to the text elements. I've changed the date line-height to match the text elements so they all visually appear to have the same baseline and let the icons settle on the center of the vertical (having also had to tweak the stream icon a bit).
The text elements maintain their alignment even if the system requests they be rendered larger, and the icons don't look terrible where they are placed:

Cool, that solution sounds good to me.

If Flutter's Row accepted an option like CrossAxisAlignment.baselineCenter, so that it aligned to the baseline but fell back to center instead of to start, then that would be cleaner because it'd let us get the same effect as you have there but without having to mess with the line-height. But alas.

Alternatively if there were a widget that were akin to IgnoreBaseline but it moved the baseline around, rather than erase it, then we could use CrossAxisAlignment.baseline and use that widget to adjust where the icons sit vertically. That'd be a slightly different result, which might be better or worse. If that need comes up again, I may write such a widget — it'll require writing a render object, but shouldn't be a lot of code.

gnprice · 2023-11-27T01:18:12Z

lib/widgets/message_list.dart

+              padding: const EdgeInsets.symmetric(vertical: 11),
+              child: Text(stream?.name ?? message.displayRecipient, // TODO(log) if missing
+                style: textStyle,
+                maxLines: 1,


Does this maxLines line do anything? It seems like it doesn't.

gnprice · 2023-11-27T01:28:02Z

test/widgets/message_list_test.dart

+        final stream = eg.stream(isWebPublic: false, inviteOnly: false);
+        await setupMessageListPage(tester,
+          messages: [message], streams: [stream]);


This test is fragile because it relies on message being to the stream stream, and that's true only by accident. (It's true only because eg.stream currently uses a constant stream ID every time by default, whereas we ought to instead make it generate a distinct random ID every time.)

Can instead write messages: [eg.streamMessage(stream: stream)].

sirpengi · 2023-11-28T23:21:33Z

@gnprice Ready for review again!

gnprice

Thanks @sirpengi for the revision! This all now looks good. Just a couple of small comments.

gnprice · 2023-11-28T23:28:11Z

lib/widgets/message_list.dart

+                  // TODO: Give a way to see the whole topic (maybe a
+                  //   long-press interaction?)
+                  style: textStyle,
+                  overflow: TextOverflow.ellipsis))),


This comment is about the overflow parameter, so swapping style and overflow separates the comment from what it's about.

There's no change to the overflow value, so can instead just leave it in place and only touch the style line.

gnprice · 2023-11-28T23:38:00Z

lib/widgets/message_list.dart

+          // In Figma this has a line-height of 19, but using 18
+          // here to match the stream/topic text widgets helps
+          // to align all the text to the same baseline.
+          height: (18 / 16),


This lgtm; I believe the line-height doesn't matter here (on the datestamp) except as a means to get it properly vertically aligned, and this seems like a good way to do that. @terpimost FYI.

gnprice · 2023-11-28T23:50:19Z

lib/widgets/message_list.dart

+                // A null [Icon.icon] makes a blank space.
+                (stream != null) ? iconDataForStream(stream) : null)),
+            Padding(
+              padding: const EdgeInsets.symmetric(vertical: 11),


Cool, that solution sounds good to me.

If Flutter's Row accepted an option like CrossAxisAlignment.baselineCenter, so that it aligned to the baseline but fell back to center instead of to start, then that would be cleaner because it'd let us get the same effect as you have there but without having to mess with the line-height. But alas.

Alternatively if there were a widget that were akin to IgnoreBaseline but it moved the baseline around, rather than erase it, then we could use CrossAxisAlignment.baseline and use that widget to adjust where the icons sit vertically. That'd be a slightly different result, which might be better or worse. If that need comes up again, I may write such a widget — it'll require writing a render object, but shouldn't be a lot of code.

gnprice · 2023-11-28T23:59:00Z

lib/widgets/message_list.dart

+              // Figma shows 5px spacing but 6px here matches better visually.
+              padding: const EdgeInsets.symmetric(horizontal: 6, vertical: 11),


Hmm I see — and our hash_sign.svg has the tips of the strokes actually touching the edge of the canvas:

so it corresponds to that 16x16 area in the middle. That's the discrepancy, then — glad to have an explanation for it. @terpimost FYI.

sirpengi · 2023-11-29T10:51:23Z

@gnprice ready again!

Taken today from Figma: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=41%3A9095&mode=dev

First in a series of changes to update stream/topic recipient headers. Stream name is now only displayed in `AllMessagesNarrow` and clicking this name navigates to the stream. Renamed `StreamTopicRecipientHeader` to `StreamMessageRecipientHeader` to reflect this conceptual change. Also adjusted existing test for unknown streams to explicitly use `AllMessagesNarrow`. Added doc links to design specs and upcoming commits will incrementally bring these headers closer to the desired design.

Design specifies 16px horizontal padding in recipient headers, see zulip/zulip-mobile#5511 . Also adjusted message timestamps so they line up with the new headers.

Also prepares header dates to receive a color from its parent so it can be changed depending on context.

Switch from colored background with chevron shaped border to inline chevron icon.

Fixes: zulip#372

Fixes: zulip#220

gnprice · 2023-11-29T19:41:26Z

Thanks for the revision!

Merging, with one tweak to make the comment about the icon spacing reflect the explanation we found in #416 (comment) :

-              // Figma specifies 5px horizontal spacing but 6px matches
-              // more accurately what we see in Figma.
+              // Figma specifies 5px horizontal spacing around an icon that's
+              // 18x18 and includes 1px padding.  The icon SVG is flush with
+              // the edges, so make it 16x16 with 6px horizontal padding.

gnprice · 2023-11-29T19:43:15Z

(Also tweaked the PR description so it's marked as closing #220.)

sirpengi requested review from gnprice and chrisbobbe November 24, 2023 18:42

gnprice reviewed Nov 27, 2023

View reviewed changes

sirpengi force-pushed the pr-new-stream-headers branch 3 times, most recently from cd65a2b to c32e5ff Compare November 28, 2023 23:19

gnprice reviewed Nov 29, 2023

View reviewed changes

sirpengi force-pushed the pr-new-stream-headers branch from c32e5ff to 5464033 Compare November 29, 2023 09:50

sirpengi mentioned this pull request Nov 29, 2023

Redesigned DM recipient headers #426

Merged

sirpengi added 9 commits November 29, 2023 14:16

icons: Add chevron_right custom icon

fc7400e

Taken today from Figma: https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=41%3A9095&mode=dev

msglist: AppBars for subscribed streams use subscription color

c1fe04b

ui: Maintain consistent color in AppBar

82eb41e

msglist: Adjust horizontal padding in recipient headers

2842951

Design specifies 16px horizontal padding in recipient headers, see zulip/zulip-mobile#5511 . Also adjusted message timestamps so they line up with the new headers.

msglist: Style recipient header dates according to design

37e478d

Also prepares header dates to receive a color from its parent so it can be changed depending on context.

msglist: Update chevron design in StreamMessageRecipientHeader

a56577a

Switch from colored background with chevron shaped border to inline chevron icon.

msglist: Apply subscription color to StreamMessageRecipientHeader

e97e6fd

msglist: Apply text style and padding to stream/topic recipient headers

1a4f0da

Fixes: zulip#372

sirpengi force-pushed the pr-new-stream-headers branch from 5464033 to b9df04e Compare November 29, 2023 15:27

msglist: Add stream icon to recipient headers

32ce11a

Fixes: zulip#220

gnprice force-pushed the pr-new-stream-headers branch from b9df04e to 32ce11a Compare November 29, 2023 19:41

gnprice merged commit 32ce11a into zulip:main Nov 29, 2023

sirpengi deleted the pr-new-stream-headers branch January 23, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesigned stream/topic recipient headers #416

Redesigned stream/topic recipient headers #416

sirpengi commented Nov 24, 2023 •

edited by gnprice

Loading

sirpengi commented Nov 24, 2023

gnprice left a comment

gnprice Nov 27, 2023

sirpengi Nov 27, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

sirpengi Nov 27, 2023

gnprice Nov 27, 2023

sirpengi Nov 28, 2023

gnprice Nov 28, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

sirpengi Nov 27, 2023

sirpengi Nov 28, 2023

gnprice Nov 28, 2023

gnprice Nov 27, 2023

gnprice Nov 27, 2023

sirpengi commented Nov 28, 2023

gnprice left a comment

gnprice Nov 28, 2023

gnprice Nov 28, 2023

gnprice Nov 28, 2023

gnprice Nov 28, 2023

sirpengi commented Nov 29, 2023

gnprice commented Nov 29, 2023

gnprice commented Nov 29, 2023

	final streamWidget = (showStream)
	final streamWidget = showStream

		// Figma shows 5px spacing but 6px here matches better visually.
		padding: const EdgeInsets.symmetric(horizontal: 6, vertical: 11),

Redesigned stream/topic recipient headers #416

Redesigned stream/topic recipient headers #416

Conversation

sirpengi commented Nov 24, 2023 • edited by gnprice Loading

sirpengi commented Nov 24, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 28, 2023

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirpengi commented Nov 29, 2023

gnprice commented Nov 29, 2023

gnprice commented Nov 29, 2023

sirpengi commented Nov 24, 2023 •

edited by gnprice

Loading