Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Star and unstar messages; show starred status #449

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

sirpengi
Copy link
Contributor

@sirpengi sirpengi commented Dec 13, 2023

This PR is built on top of #446 .

The alignment of the star marker is currently only ideal for default text (see screenshots at bottom). Unfortunately some markup introduce large margins, so the alignment is not attractive for a message that starts with a header:

star_on_header

Ideally the top margins of messages are collapsed, but that is outside the scope of this PR.

In this PR the star is offset from the top by 4px, which more or less matches visually what the Figma at https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=813%3A28811&mode=dev tries to achieve (the star shape centered with the first line of text), but will need to be readjusted (in addition to the margin fix) when we match the default text style to the guide (we currently use 14px and the mockups use 17).

Fixes: #170

starred_message

action_sheet

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sirpengi for building this!

This review is for the last three commits:
4d352bb icons: Add star custom icon
85bc0ff msglist: Add star marker in messages
4a6c46b action_sheet: Add button to "star" and "unstar" message
as the rest are in #446.

Generally this looks good. Comments below, mostly small.

<svg
width="22"
height="22"
viewBox="0 0 22 22"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icons: Add star custom icon

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

When I visit that URL… well, first, it seems to be a list of icons and not a specific icon. But when I choose the one star icon I see there, I get an SVG that begins differently and in particular is 24x24:
image

That 24x24 version differs by having some internal padding; this 22x22 version has the star go all the way to the edge. Also the 24x24 version is hollow, while this one is filled.

Can you spell out in more detail how you got this version?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively: how did you get to that URL from the design for the screen? The design for the screen that has a starred message (the one I'm looking at that's linked to from #170, anyway) is here:
https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=385%3A12861&mode=dev
and it looks like it uses a different icon file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The figma is organized into pages with a specific icons page. From my sense of things, it's the canonical list of icons: git log assets/icons shows that the majority of icons are pulled from this page, there's one counterexample that was delivered over CZO.
Because we needed a filled-version over an outlined-version, I put the version from the icons page through inkscape and did a conversion there (using stroke-to-path to expand the outline and then keeping only the outer shape). The metadata in the svg of star.svg betrays that it was output from inkscape, and an existing read_receipts.svg shows that it was also made/altered in inkscape.
I think comparing visually they all appear the same. I don't think the defined size in the svg makes a difference in the icon font (read_receipts.svg has a height/width of 512, and hash_sign.svg has 1024).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we needed a filled-version over an outlined-version, I put the version from the icons page through inkscape and did a conversion there (using stroke-to-path to expand the outline and then keeping only the outer shape).

I see. Please definitely include information like that when sending an icon in a PR — that seems like the kind of operation that @terpimost is likely to want to review, or to do himself.

More generally, if you got a thing from a place but then made some changes to it, please don't just say you got it from there and not mention the changes.

When I tried downloading the same icon from the place you said and got a different result, I spent some time (as you see above) spelling out exactly how I got it, because I figured there was a discrepancy we needed to debug between two different ways of getting an icon from Figma. It would have saved me that time to know that the file in the PR wasn't something one could find in Figma at all.

I think comparing visually they all appear the same.

I don't understand this part. What looks the same as what? The icons in that directory are all different from each other, naturally, and you've just described how you made this icon different from the one at the page you linked to.

git log assets/icons shows that the majority of icons are pulled from this page, there's one counterexample that was delivered over CZO.

(Not that it's the main point, but this is not true: try adding --stat to that command and you'll see why. The majority of the icons all come from the first commit there. It looks like I didn't specify this in the commit message, but those icons came from Zulip web.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood about documenting any modifications, I will make sure to do that from now.

I think comparing visually they all appear the same.

What I meant was they all had the same features, rounded corners and a sort of bulged shape, and so appeared to be the same shape when filled (despite the differences in the svg). I've changed it now to source from Zulip web as well, and noted that zulip/zulip#25903 is the original source of the filled version.

child: message.flags.contains(MessageFlag.starred)
? Icon(ZulipIcons.star,
size: 16,
color: const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's pull this out as a static final, so it only gets computed once. E.g. static final _starColor = ….

? Icon(ZulipIcons.star,
size: 16,
color: const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor())
: const SizedBox.shrink()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
: const SizedBox.shrink()),
: null),

Comment on lines 850 to 853
SizedBox(width: 16,
child: message.flags.contains(MessageFlag.starred)
? Icon(ZulipIcons.star,
size: 16,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid specifying the width twice — what happens in the starred case if we drop the SizedBox?

If that works, then we can drop the outer SizedBox, and just have a SizedBox in the not-starred case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't able to sort out a way to not specify the width twice by dropping the SizedBox. To maintain the layout of the gutter in the not-starred case the width would need to be specified in the replacement element.

On further exploration I've found FittedBox which assists in removing the duplication of the width:

SizedBox(width: 16,
  child: message.flags.contains(MessageFlag.starred)
    ? FittedBox(child:
        Icon(ZulipIcons.star, color: _kStarColor))
    : null)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. My gut reaction to FittedBox is that it sounds complicated — which shouldn't stop us from using it when what we need is to do the job it does, but makes it overkill for a small-scale code deduplication like this. So we can stick with the previous revision's SizedBox approach.

@@ -44,6 +45,7 @@ void showMessageActionSheet({required BuildContext context, required Message mes
messageListContext: context,
),
CopyButton(message: message, messageListContext: context),
StarButton(message: message, messageListContext: context),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this just after the thumbs-up reaction option. That's the one that feels most similar; and those two are also the two actions we want to make most prominent. (E.g. they're the two that appear directly on hover in Zulip web, while other actions are inside an overflow menu.)

Copy link
Contributor Author

@sirpengi sirpengi Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was copied from the order of the mobile app. But agree with the improvement and I've made the hange per your suggestion.

])),
SizedBox(width: 16,
child: message.flags.contains(MessageFlag.starred)
? Icon(ZulipIcons.star,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This star appears at the top right of the message. In Vlad's design it looks like it's at the bottom right:
image

I guess I'm not 100% sure that's meant as one message — maybe the idea is that that's two messages, and the star is at the top of the second one. @terpimost, can you confirm which you intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coming off of
193037429-aeb6af5d-7d93-43db-bcc9-809ddbab5fd7
from zulip/zulip-mobile#5511 it shows the star is aligned at the top.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, sounds good.

Would you post a screenshot from this PR where the text is long enough that it gets near the right edge? That would help in comparing the vertical alignment of the content vs. the star.

@override get onPressed => (BuildContext context) async {
Navigator.of(context).pop();
String? errorMessage;
final op = (message.flags.contains(MessageFlag.starred))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: redundant parens

op: op, flag: MessageFlag.starred);
} catch (e) {
if (!messageListContext.mounted) return;
final zulipLocalizations = ZulipLocalizations.of(messageListContext);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move this down next to the showErrorDialog call where it's used

(just before used, or at the top of the function, can both work well for something like this; but here feels like an odd hybrid)

Comment on lines 363 to 364
UpdateMessageFlagsOp.add => zulipLocalizations.errorStarMessageFailedTitle,
UpdateMessageFlagsOp.remove => zulipLocalizations.errorUnstarMessageFailedTitle,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: match order here with the previous two conditionals between these cases — makes it a bit easier for the reader

Comment on lines 396 to 400
// Locating button by the icon (as other tests here do) is complicated
// by the fact that starred messages are decorated with the same icon.
// Selecting by button class instead here:
await tester.ensureVisible(find.byType(StarButton, skipOffstage: false));
await tester.tap(find.byType(StarButton));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see.

I think in my ideal version of this code, StarButton and its siblings would be private — they're part of the internal organization of action_sheet.dart but not part of its interface.

How about looking for the icon, but within the descendants of a BottomSheet? Can use find.descendant. If it's hard to make that work, this version is OK; but I suspect it'll be easy.

@sirpengi sirpengi force-pushed the pr-starring-messages branch 3 times, most recently from 2684bae to b9a9774 Compare January 11, 2024 14:51
@sirpengi
Copy link
Contributor Author

@gnprice ready for review! Changed and documented the source of the filled star icon, and made small adjustments per your comments. The screenshots were updated and I've noted in the PR where the alignment looks unattractive when paired with headers, but I think that is outside the scope of this.

}
}

final _kStarColor = const HSLColor.fromAHSL(0.5, 47, 1, 0.41).toColor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: better would be a static final on the class, as I suggested at #449 (comment) . That way the class helps organize this part of the code too.

Comment on lines 60 to 61
/// The Zulip custom icon "star".
static const IconData star = IconData(0xf10c, fontFamily: "Zulip Icons");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icons: Add star custom icon

This is a filled version of the Feather star icon.
Copied from zulip web `web/shared/icons/star-filled.svg`
which sourced it from:
  https://github.com/zulip/zulip/issues/25903

We're implementing a specific design that's intended for mobile. That design includes an icon, which I pointed to here:
#449 (comment)

So we should use the icon from the design we're intending to implement. A similar icon in web might happen to be the same, or it might not — sometimes there are differences because it's a different context.

Comment on lines 351 to 354
if (!messageListContext.mounted) return;
String? errorMessage;

switch (e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this errorMessage variable doesn't have anything to do with the if-unmounted-return, but is related to the switch because that's what can set it. So it's weird to have it attached to the former and separated from the latter.

Suggested change
if (!messageListContext.mounted) return;
String? errorMessage;
switch (e) {
if (!messageListContext.mounted) return;
String? errorMessage;
switch (e) {

Comment on lines 918 to 922
// TODO(#157): fix how star marker aligns with message content
// Design from Figma at:
// https://www.figma.com/file/1JTNtYo9memgW7vV6d0ygq/Zulip-Mobile?node-id=813%3A28817&mode=dev .
? Padding(padding: const EdgeInsets.only(top: 4),
child: FittedBox(child: Icon(ZulipIcons.star, color: _kStarColor)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the issue mentioned at the top of the PR description, about headings in the message, it seems like the star is too high even when the message starts with a paragraph.

In your screenshot:
image
basically the whole top lobe of the star sticks out above the ascenders of the text. But in the image you quoted at #449 (comment) from Vlad's design, the upper tip of the star is pretty much right at the ascender line.

So that would be good to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a side by side, left is the app and right is the mockup:

side_by_side

It looks to me that the star is vertically aligned with the text (there's the same-ish space above and below the h) and that's the approach I took to aligning the star with the paragraph.

In the process of making the screenshot I did end up moving the star up 1px so the padding is now top: 3px (I was tweaking the alignment based on the numeral 2 but it seems the h is a little bit taller). It's not perfectly centered but it's much closer than the previous top: 4px.

Copy link
Contributor Author

@sirpengi sirpengi Jan 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one other reason the star appears to ascend even higher is that our unstyled text is currently 14px (see kbaseFontSize as well as Paragraph in lib/widgets/contents.dart) whereas the mockup has 17px. This change is incoming - I noticed this during my work on #157. There's already a comment in the code noting that we'll need to revisit this alignment when the content styles are updated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Playing with it, the star still feels slighly misaligned to me, and I think a 4px offset (rather than 3px) comes out better.

I think the key to how it looks for me is the shoulders of the star, i.e. the upper sides of the left and right tines — in the current revision those are noticeably higher than the x-height of the text (which is the top of most of the letters), whereas in the mockup it's right in line. Moving it another pixel down fixes that.

@@ -317,3 +319,51 @@ class CopyButton extends MessageActionSheetMenuItemButton {
data: ClipboardData(text: rawContent));
};
}

class StarButton extends MessageActionSheetMenuItemButton {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put the classes in the same order as the buttons appear in the UI (and in the list up in showMessageActionSheet. That will help when navigating the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also changed the order in the tests to match!

Comment on lines 850 to 853
SizedBox(width: 16,
child: message.flags.contains(MessageFlag.starred)
? Icon(ZulipIcons.star,
size: 16,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. My gut reaction to FittedBox is that it sounds complicated — which shouldn't stop us from using it when what we need is to do the job it does, but makes it overkill for a small-scale code deduplication like this. So we can stick with the previous revision's SizedBox approach.

Copy link
Member

@gnprice gnprice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the revision! Comments above, because GitHub.

@sirpengi
Copy link
Contributor Author

@gnprice ready for review again!

@gnprice
Copy link
Member

gnprice commented Jan 23, 2024

Thanks for the revision! All looks good to me now modulo a small tweak: #449 (comment)

Otherwise this is just pending #446 getting finished and merged.

@@ -57,14 +57,17 @@ abstract final class ZulipIcons {
/// The Zulip custom icon "read_receipts".
static const IconData read_receipts = IconData(0xf10b, fontFamily: "Zulip Icons");

/// The Zulip custom icon "star".
static const IconData star = IconData(0xf10c, fontFamily: "Zulip Icons");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, one other comment: let's call this icon star_filled. That way it matches the name Vlad gave it in the mockup, and it makes the name more descriptive since we may have an unfilled star in the future.

@sirpengi sirpengi force-pushed the pr-starring-messages branch 2 times, most recently from 959c2ad to f5b904c Compare January 23, 2024 13:29
@sirpengi
Copy link
Contributor Author

@gnprice thanks for the review! comments have been addressed and I noticed there weren't tests for the visibility of the icon in the message so I added those as well.

@gnprice
Copy link
Member

gnprice commented Jan 24, 2024

Cool, thanks. All LGTM now pending #446.

@gnprice
Copy link
Member

gnprice commented Jan 25, 2024

#446 has been merged; rebasing this and merging.

@gnprice gnprice merged commit fd9e858 into zulip:main Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Star and unstar messages; show starred status
2 participants