msglist: Implement new edited/moved label design. #900

PIG208 · 2024-08-20T21:37:23Z

This replaces the gutter edit state marker design. Since it leads to message displacement, a previous test case is no longer applicable.

We also inline part of what was previously in edit_state_marker.dart, because the new design simplifies the implementation.

Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3038-56393&t=WMT80mwuFaruNbVf-1

Fixes: #171

PIG208 · 2024-08-20T21:38:30Z

Wasn't able to reopen #765 after accidentally closing it:

Here's the new PR for the new design.

PIG208 · 2024-08-20T21:45:17Z

Screenshots

light	dark

PIG208 · 2024-08-21T20:42:04Z

Rebased and simplified the implementation a bit.

gnprice · 2024-08-22T00:05:05Z

Wasn't able to reopen #765 after accidentally closing it:

FWIW for next time: I think the solution there is to push the commits you want back to the PR's branch, and then re-open the PR.

(In particular the error message there says "There are no new commits on the … branch", i.e. the branch is empty compared to main. You can fix that by pushing to fix what's in the branch.)

gnprice · 2024-08-22T00:05:45Z

Though in this case, since the implementation is radically different from #765, a fresh PR is probably cleanest anyway.

PIG208 · 2024-08-22T00:23:44Z

FWIW for next time: I think the solution there is to push the commits you want back to the PR's branch, and then re-open the PR.

I do recall running into the same issue a few years back. Couldn't find the link right now.

My attempted fix this time was amending the latest commit, and adding an empty commit on top of it. It's interesting that it didn't work. Will look into this next time (without mistakenly pushing to upstream!)

chrisbobbe

Thanks! Small comments below.

chrisbobbe · 2024-08-23T22:06:57Z

lib/widgets/message_list.dart

@@ -86,7 +84,7 @@ class MessageListTheme extends ThemeExtension<MessageListTheme> {
    required this.dateSeparator,
    required this.dateSeparatorText,
    required this.dmRecipientHeaderBg,
-    required this.editedMovedMarkerCollapsed,
+    required this.editStateLabel,


It looks like there's a Figma design variable for this, labelEdited, so let's use that and put it in DesignVariables, sorted alphabetically in the default section (not marked // Not exactly from the Figma design […] or // Not named variables in Figma […]).

chrisbobbe · 2024-08-23T22:13:17Z

lib/widgets/message_list.dart

                  if ((message.reactions?.total ?? 0) > 0)
-                    ReactionChipsList(messageId: message.id, reactions: message.reactions!)
+                    ReactionChipsList(messageId: message.id, reactions: message.reactions!),
+                  if (editStateText != null)
+                    Text(editStateText, textAlign: TextAlign.end,
+                      style: TextStyle(
+                        color: messageListTheme.editStateLabel,
+                        fontFamily: 'Source Sans 3',
+                        fontSize: 12,
+                        height: (12 / 12))),


I guess the Figma isn't clear whether the reaction chips or the marker should come first, vertically. How about putting the marker first? In the case where there are dozens of reaction chips that wrap onto many lines, I think it would be neater and clearer if the EDITED or MOVED marker were above them instead of below them.

Yeah that sounds good to me.

Just to be sure, started a disccusion on CZO.

chrisbobbe · 2024-08-23T22:27:16Z

lib/widgets/message_list.dart

+                    Text(editStateText, textAlign: TextAlign.end,
+                      style: TextStyle(
+                        color: messageListTheme.editStateLabel,
+                        fontFamily: 'Source Sans 3',


I think we'll get 'Source Sans 3' automatically here, like in most other places, because this is rendered on a Material (via Scaffold, I think), and Material applies TextTheme.bodyMedium to its children via AnimatedDefaultTextStyle, and we've set TextTheme.bodyMedium to include 'Source Sans 3' (see zulipTypography in lib/widgets/text.dart).

Looks like we're also redundantly passing 'Source Sans 3' where we render the sender name in MessageWithPossibleSender. Removing that (perhaps with a link to my explanation here, or quoting/paraphrasing it) would be a helpful cleanup in a separate NFC commit.

chrisbobbe · 2024-08-23T22:35:58Z

lib/widgets/message_list.dart

+                        color: messageListTheme.editStateLabel,
+                        fontFamily: 'Source Sans 3',
+                        fontSize: 12,
+                        height: (12 / 12))),


Suggested change

height: (12 / 12))),

height: (12 / 12),

letterSpacing: proportionalLetterSpacing(context, 0.05, baseFontSize: 12),

I notice the Figma has 0.6px letter spacing, but if we use proportionalLetterSpacing, it'll grow and shrink with the system font-size setting, and that seems like a nice refinement.

Thanks for catching that! Didn't know that we have proportionalLetterSpacing.

Looks like 0.6px comes from the dev mode typography section, but the 5% letter spacing is probably intended:

Oh interesting, so yeah, it looks like the 5% is expressed in the Figma too :) thanks for noticing that.

chrisbobbe · 2024-08-23T22:39:23Z

assets/l10n/app_en.arb

+  },
+  "messageIsEditedLabel": "EDITED",
+  "@messageIsEditedLabel": {
+    "description": "Label for an edited message."


Suggested change

"description": "Label for an edited message."

"description": "Label for an edited message. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)"

(similarly below)

One thing I was wondering about when implementing this, is that whether we should programmatically upper case the letters and translate the label normally, or just do it the current way.

We do have a precedent: "loginMethodDivider": "OR". But I can also see the appeal to using i18n.toUpperCase for something purely for styling purposes (similar to the text-transform: uppercase; approach as one could do in web), because in this case we aren't using uppercase letters to convey emphasis/exaggeration in the writing.

Started a discussion here. My guess is that it might be preferred to just use uppercase letters, but I'm curious if there are better ways.

PIG208 · 2024-08-26T18:26:03Z

Screenshots

light	dart

We supplied the font 'Source Sans 3' to `TextTheme.bodyMedium` in `lib/widgets/text.dart`. When rendered, `Material` applies this font via `AnimatedDefaultTextStyle` by default, so there is no need to override `fontFamily` explicitly. See: zulip#900 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2024-08-26T19:15:48Z

Implemented the suggested changes. Thanks for the review @chrisbobbe! Started some CZO discussions on things that I have questions about.

chrisbobbe · 2024-08-27T04:23:37Z

Thanks! This LGTM except the CZO discussion seems like it's settling on having the marker come after the reactions list instead of before. Marking for Greg's review once he's back from vacation.

PIG208 · 2024-08-27T04:36:30Z

Pushed to move the marker after the reactions list.

PIG208 · 2024-09-05T03:34:57Z

Updated the PR to share the final row between reaction chips and the edit state label mentioned here.

If necessary, we could add an RTL test case later for the new commit if we like this approach.

PIG208 · 2024-09-05T03:36:31Z

Preview

Space saving

Before	After

Overlap prevention

Barely enough space	Not enough space	Not enough space (w/o the workaround)

We supplied the font 'Source Sans 3' to `TextTheme.bodyMedium` in `lib/widgets/text.dart`. When rendered, `Material` applies this font via `AnimatedDefaultTextStyle` by default, so there is no need to override `fontFamily` explicitly. See: zulip#900 (comment) Signed-off-by: Zixuan James Li <[email protected]>

gnprice

Thanks @PIG208, and thanks @chrisbobbe for the previous reviews!

Let's skip for now the complexity of trying to squash the edited/moved marker onto the same line as the end of the message. I think this version doesn't yet quite work — see below — and I'm also a little concerned about making the layout complicated, if for example the user is reading a long thread that got moved and so all the messages have markers. If people really find the extra vertical space annoying, we can revisit that.

Otherwise this all looks good, with just one nit below.

gnprice · 2024-09-13T08:41:54Z

lib/widgets/message_list.dart

+                          trailingPlaceholder: editStateLabel),
+                    ]),
+                  if (editStateLabel != null)
+                    PositionedDirectional(bottom: 0, end: 0, child: editStateLabel),


What happens if there are no reactions, but the message's own text ends with a full line? I think in this version that will end up having an overlap of the "EDITED" marker and the message text.

Hmm, yeah, that could totally happen. It does look like that furthering this implementation to fix that as well can make it more complicated, unless we get a different solution.

gnprice · 2024-09-13T08:47:36Z

lib/widgets/message_list.dart

+    final editStateLabel = editStateText == null ? null : Text(editStateText,
+      textAlign: TextAlign.end,


nit:

Suggested change

final editStateLabel = editStateText == null ? null : Text(editStateText,

textAlign: TextAlign.end,

final editStateLabel = editStateText == null ? null

: Text(editStateText,

textAlign: TextAlign.end,

That way the Text constructor name, and editStateText which holds the actual text content, stay more visible and next to the other arguments to Text.

Starting textAlign in a new line for the now inlined editStateLabel because we don't need to render another copy now.

PIG208 · 2024-09-13T14:01:55Z

Dropped the commit 783b5a2, and pushed an update. Thanks!

We supplied the font 'Source Sans 3' to `TextTheme.bodyMedium` in `lib/widgets/text.dart`. When rendered, `Material` applies this font via `AnimatedDefaultTextStyle` by default, so there is no need to override `fontFamily` explicitly. See: zulip#900 (comment) Signed-off-by: Zixuan James Li <[email protected]>

This replaces the gutter edit state marker design. Since it leads to message displacement, the test "edit state updates do not affect layout" is no longer applicable. We also inline part of what was previously in edit_state_marker.dart, because the new design simplifies the implementation, and remove the now unused icons. The design variables are taken from the Figma design: https://www.figma.com/design/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=3038-56393&t=WMT80mwuFaruNbVf-1 Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2024-09-16T23:15:26Z

Thanks! Looks good; merging.

PIG208 force-pushed the marker-ui-final branch from 1bc2f50 to fe45371 Compare August 20, 2024 21:46

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Aug 20, 2024

PIG208 force-pushed the marker-ui-final branch 4 times, most recently from 85e0fb9 to 3ce5a96 Compare August 20, 2024 21:57

PIG208 requested a review from chrisbobbe August 20, 2024 21:58

PIG208 force-pushed the marker-ui-final branch from 3ce5a96 to 0fcc79a Compare August 21, 2024 20:41

chrisbobbe reviewed Aug 23, 2024

View reviewed changes

PIG208 force-pushed the marker-ui-final branch 2 times, most recently from f976938 to 9a0f7da Compare August 26, 2024 18:44

PIG208 requested a review from chrisbobbe August 26, 2024 19:10

PIG208 force-pushed the marker-ui-final branch from 9a0f7da to b292724 Compare August 26, 2024 19:13

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Aug 27, 2024

PIG208 force-pushed the marker-ui-final branch from b292724 to bcac0a2 Compare August 27, 2024 04:36

PIG208 force-pushed the marker-ui-final branch from bcac0a2 to f7ab0ba Compare August 27, 2024 04:37

PIG208 force-pushed the marker-ui-final branch from b560fc0 to 1f5ea3d Compare September 5, 2024 03:32

PIG208 force-pushed the marker-ui-final branch from 1f5ea3d to 45f91aa Compare September 7, 2024 04:24

PIG208 requested review from gnprice and removed request for chrisbobbe September 11, 2024 17:05

PIG208 force-pushed the marker-ui-final branch 3 times, most recently from 9c52563 to 783b5a2 Compare September 12, 2024 22:08

gnprice reviewed Sep 13, 2024

View reviewed changes

PIG208 force-pushed the marker-ui-final branch from 783b5a2 to cb72ff1 Compare September 13, 2024 13:56

PIG208 added 2 commits September 16, 2024 16:14

gnprice force-pushed the marker-ui-final branch from cb72ff1 to 6ca5524 Compare September 16, 2024 23:15

gnprice merged commit 6ca5524 into zulip:main Sep 16, 2024
1 check passed

PIG208 deleted the marker-ui-final branch September 17, 2024 00:18

gnprice mentioned this pull request Oct 8, 2024

Show edited/moved marker on messages #171

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

msglist: Implement new edited/moved label design. #900

msglist: Implement new edited/moved label design. #900

PIG208 commented Aug 20, 2024 •

edited by gnprice

Loading

PIG208 commented Aug 20, 2024

PIG208 commented Aug 20, 2024

PIG208 commented Aug 21, 2024 •

edited

Loading

gnprice commented Aug 22, 2024

gnprice commented Aug 22, 2024

PIG208 commented Aug 22, 2024

chrisbobbe left a comment

chrisbobbe Aug 23, 2024

chrisbobbe Aug 23, 2024

PIG208 Aug 26, 2024

PIG208 Aug 26, 2024

chrisbobbe Aug 23, 2024

chrisbobbe Aug 23, 2024

chrisbobbe Aug 23, 2024

PIG208 Aug 26, 2024

chrisbobbe Aug 27, 2024

chrisbobbe Aug 23, 2024

PIG208 Aug 26, 2024

PIG208 Aug 26, 2024

PIG208 commented Aug 26, 2024

PIG208 commented Aug 26, 2024

chrisbobbe commented Aug 27, 2024

PIG208 commented Aug 27, 2024

PIG208 commented Sep 5, 2024

PIG208 commented Sep 5, 2024 •

edited

Loading

Space saving

Overlap prevention

gnprice left a comment

gnprice Sep 13, 2024

PIG208 Sep 13, 2024

gnprice Sep 13, 2024

PIG208 Sep 13, 2024 •

edited

Loading

PIG208 commented Sep 13, 2024

gnprice commented Sep 16, 2024

	height: (12 / 12))),
	height: (12 / 12),
	letterSpacing: proportionalLetterSpacing(context, 0.05, baseFontSize: 12),

	"description": "Label for an edited message."
	"description": "Label for an edited message. (Use ALL CAPS for cased alphabets: Latin, Greek, Cyrillic, etc.)"

		final editStateLabel = editStateText == null ? null : Text(editStateText,
		textAlign: TextAlign.end,

msglist: Implement new edited/moved label design. #900

msglist: Implement new edited/moved label design. #900

Conversation

PIG208 commented Aug 20, 2024 • edited by gnprice Loading

PIG208 commented Aug 20, 2024

PIG208 commented Aug 20, 2024

PIG208 commented Aug 21, 2024 • edited Loading

gnprice commented Aug 22, 2024

gnprice commented Aug 22, 2024

PIG208 commented Aug 22, 2024

chrisbobbe 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

PIG208 commented Aug 26, 2024

PIG208 commented Aug 26, 2024

chrisbobbe commented Aug 27, 2024

PIG208 commented Aug 27, 2024

PIG208 commented Sep 5, 2024

PIG208 commented Sep 5, 2024 • edited Loading

Space saving

Overlap prevention

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

PIG208 Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Sep 13, 2024

gnprice commented Sep 16, 2024

PIG208 commented Aug 20, 2024 •

edited by gnprice

Loading

PIG208 commented Aug 21, 2024 •

edited

Loading

PIG208 commented Sep 5, 2024 •

edited

Loading

PIG208 Sep 13, 2024 •

edited

Loading