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

Support adding arbitrary reactions #1103

Merged
merged 3 commits into from
Dec 11, 2024

Conversation

rajveermalviya
Copy link
Collaborator

@rajveermalviya rajveermalviya commented Dec 4, 2024

Action sheet Action sheet (with reactions) Emoji picker
Screenshot 2024-12-07 at 02 12 44 Screenshot 2024-12-07 at 02 13 02 Screenshot 2024-12-07 at 02 13 11
Screenshot 2024-12-07 at 02 12 41 Screenshot 2024-12-07 at 02 12 58 Screenshot 2024-12-07 at 02 13 09

Fixes: #388

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch 3 times, most recently from 7d104af to e74f502 Compare December 4, 2024 20:13
}

// Adapted from web/src/emoji_picker.ts
void _filterEmojis(String query) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have this use the emoji autocomplete machinery instead, so it works the same as in the compose box.

That has some useful features this version doesn't; and I think it's also useful just for them to feel consistent with each other for the user.

Copy link
Member

Choose a reason for hiding this comment

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

For excluding the popular and already-reacted emoji, I think a good approach would be to extend EmojiAutocompleteQuery so it takes a parameter with a set of emoji to exclude.

But I'd also be fine with not handling that exclusion in this PR, and making it an M5 or M6 follow-up task. I think it's pretty OK if those emoji appear in the list.

@@ -30,7 +30,14 @@ class UnicodeEmojiDisplay extends EmojiDisplay {
/// The actual Unicode text representing this emoji; for example, "🙂".
final String emojiUnicode;

UnicodeEmojiDisplay({required super.emojiName, required this.emojiUnicode});
/// The Zulip "emoji code" for this emoji; for example, "1f642" for "🙂" emoji.
final String emojiCode;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally this doesn't belong here — this class is meant to be strictly the information used for displaying the emoji.

What are the places that would need to change without this field? I suspect they can pass around an EmojiCandidate instead.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch from e74f502 to 27efb25 Compare December 4, 2024 21:22
Comment on lines 67 to 68
const popularUnicodeEmojis = [
UnicodeEmojiDisplay(
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it looks like basically this one spot needs to switch to EmojiCandidate, and then that'll supply an emojiCode at the one place that needs it, in ReactionButtons.

EmojiCandidate is the right class here conceptually too. In principle, and in the future, the emoji we show in the quick list at the top of the action sheet should depend on actual usage and may include realm emoji (or perhaps even sometimes the Zulip extra emoji, :zulip:).

@@ -71,7 +71,9 @@ class UnicodeEmojiWidget extends StatelessWidget {
SizedBox(height: boxSize, width: boxSize),
PositionedDirectional(start: 0, child: Text(
textScaler: textScaler,
style: TextStyle(fontSize: size),
style: TextStyle(
fontFamily: 'Apple Color Emoji',
Copy link
Member

Choose a reason for hiding this comment

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

emoji: Use "Apple Color Emoji" font on iOS/macOS for UnicodeEmojiWidget

Some unicode characters, like U+2764 (❤) or U+00AE (®) can
have glyphs in non-Emoji fonts, resulting in incorrect
rendering of such characters, where we specifically want an
emoji to be displayed.

So, explicitly mention "Apple Color Emoji" to be the font used on
iOS/macOS for displaying the unicode emoji.

Interesting — I reproduce the issue.

I think the right fix is actually somewhat broader. I'll file an issue and send a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Well, filed #1104. Then the broad fix I had in mind turned out not to work: #1104 (comment)

I have a draft branch that will fix that issue for emoji in message content, which is the symptom I described in the issue. But we'll still need exactly this fix for this UnicodeEmojiWidget class, found in reaction chips and autocomplete results. So go ahead and leave this commit in your PR; I might also cherry-pick it into the PR I send for #1104.

Copy link
Member

Choose a reason for hiding this comment

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

OK, sent #1108. It includes a cherry-pick of this commit — the only change I made to the diff was to add a test.

@gnprice
Copy link
Member

gnprice commented Dec 5, 2024

I added the "Fixes" link to the issue this is a draft PR for fixing. Same reasoning as here:
#1076 (comment)

@alya
Copy link
Collaborator

alya commented Dec 5, 2024

Let's use "more" rather than "other" for the additional reactions.

@gnprice
Copy link
Member

gnprice commented Dec 5, 2024

OK, and a couple of comments from playing with the UI just now:

  • Can we get the search box to be focused (and the keyboard to appear) as soon as the user opens the search modal? The list of options is always going to be huge, and at least at present it's going to be rare that the desired emoji is in the first screenful or two — the user's almost always going to want to type a query to search.
  • The results always show only a single name per emoji. (With the exception of before the user types anything — but as mentioned, that's going to be a small part of people's experience.) The design calls instead for showing a list of names.

@gnprice
Copy link
Member

gnprice commented Dec 5, 2024

Excited to see this! Looking forward to getting it completed and merged.

Some high-level comments above from an initial review yesterday. (After leaving individual comments piecemeal I forgot to leave an overall comment like this one.)

Our autocomplete machinery is definitely somewhat complicated, but I think it should work well for this task. If you have any questions as you go to use it, please don't hesitate to ask in #mobile-team.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch 2 times, most recently from b39060e to d8dfb00 Compare December 6, 2024 20:17
@rajveermalviya rajveermalviya marked this pull request as ready for review December 6, 2024 20:49
@rajveermalviya
Copy link
Collaborator Author

Thanks for the initial comments @gnprice @alya!
This is now ready for review, I've also updated the screenshots.

@rajveermalviya rajveermalviya added the maintainer review PR ready for review by Zulip maintainers label Dec 6, 2024
@chrisbobbe chrisbobbe self-requested a review December 6, 2024 22:36
@chrisbobbe chrisbobbe self-assigned this Dec 6, 2024
Copy link
Collaborator

@chrisbobbe chrisbobbe left a comment

Choose a reason for hiding this comment

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

Thanks! Very excited for this feature. Comments below.

Comment on lines 91 to 94
// Zulip's hand selected "popular" emojis, currently used as list of
// quick emoji reactions available in the message context menu.
// See: https://github.com/zulip/zulip/blob/3bad36ef8cf07cd57d0b257a739bae635a8527ac/web/shared/src/typeahead.ts#L22-L29
const zulipPopularEmojis = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it should be a dartdoc /// instead of implementation comment //.

Comment on lines 194 to 196
/// List of popular emoji reaction buttons to display.
/// Each emoji must be a unicode emoji.
final List<EmojiCandidate> popularEmojis;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a param for this hard-coded list, or can we just use the list directly?

}

return Container(
padding: const EdgeInsets.all(8),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The Figma doesn't put padding around the list of hard-coded reactions:

image

Instead, it gives more padding—and presumably more touchable area—to each button. Let's follow that.

For the selected/pressed state, we can just change the background color of the whole padded area. Like we do for the "more" button:

image

and the other buttons in the action sheet:

image

(I see the pressed/selected states aren't represented in the Figma, but I think my proposal should work fine.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately, adding 10px horizontal padding on each button will results in overflow on the minimum supported screen width (320px) with default text size:

horizontal: 10 horizontal: 5
Screenshot 2024-12-07 at 09 42 29 Screenshot 2024-12-07 at 09 43 04

Minimum padding that doesn't overflow is 5px.

Copy link
Collaborator Author

@rajveermalviya rajveermalviya Dec 7, 2024

Choose a reason for hiding this comment

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

Another option is to have no horizontal padding, and make the container take as much width possible and aligning the emoji in the center:

padding horizontal: 5 padding horizontal: 0, width double.infinity
Screenshot 2024-12-07 at 09 53 28 Screenshot 2024-12-07 at 09 51 58
Screenshot 2024-12-07 at 09 53 22 Screenshot 2024-12-07 at 09 51 43

Copy link
Member

Choose a reason for hiding this comment

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

That second option LGTM.

An ideal version would have a minimum width for each item, and when there isn't room for them all at that width (which can happen if the user has set their system display-size setting to make everything big) we'd just drop items from the end of the list. That way 👍 and 🎉 and perhaps 🙂 would appear at the appropriate size, at the cost of 🐙 and perhaps 🛠️ being evicted from this quick-pick line and having to be found under the "more" button instead.

But implementing that will be more complicated, so let's keep it out of scope for this PR.

Comment on lines 532 to 533
if (pageContext.mounted) Navigator.pop(pageContext); // Emoji picker
if (pageContext.mounted) Navigator.pop(pageContext); // Context menu
Copy link
Collaborator

Choose a reason for hiding this comment

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

This emoji-picker code makes an assumption about how the emoji picker was opened; that it was opened from a context menu. If that stops being true, this will pop too many routes, which will be confusing. Can we make a nice experience without making this assumption?

How about we close the message action sheet when the user presses the "more" button, like we do for the sheet's other buttons?

Comment on lines 137 to 139
await tester.tap(find.ancestor(
of: find.text(emojiDisplay.emojiUnicode),
matching: find.byType(IconButton)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the UI change I mentioned earlier (making the popular-emoji buttons fill more space) means that we don't use IconButton anymore, this test will have to change. We can't just tester.tap(find.text(emojiDisplay.emojiUnicode)) because that finder will also find any reaction chips that are present in the message list. Maybe we can filter those out, though, and tap on the remaining one, which should be this one in the action sheet?

controller: _controller,
autofocus: true,
decoration: InputDecoration(
hintText: zulipLocalizations.emojiPickerSearchEmoji,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the search input's hint text (not sure about regular input text) doesn't match the Figma. Same for the labels on the emoji results.

Comment on lines 388 to 392
// By default, when software keyboard is opened, the ListView
// expands behind the software keyboard — resulting in some
// list entries being covered by the keyboard. Add explicit
// bottom padding the size of the keyboard, which fixes this.
padding: EdgeInsets.only(bottom: MediaQuery.viewInsetsOf(context).bottom),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, odd that we have to do something here but not with the message list. I think in the message-list case we get some help from Scaffold? I see a param resizeToAvoidBottomInset there.

Your solution seems to work, and I don't see anything comparable in the showModalBottomSheet API. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

Confusing, yeah.

Let's put this next to the SafeArea widget — their jobs are closely related.

padding: const EdgeInsets.fromLTRB(12, 12, 4, 12),
splashFactory: NoSplash.splashFactory,
foregroundColor: designVariables.contextMenuItemText,
shape: const RoundedRectangleBorder(borderRadius: BorderRadius.zero)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why RoundedRectangleBorder where the rectangle is not rounded (border radius is zero)? 🙂 (Here and in another place in the PR.)

notoColorEmojiTextSize: 20.1,
size: 24));
}))))),
TextButton(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think TextButton is adding 2px padding at the top and 2px at the bottom, to make the button's height 48px instead of 44px as in the Figma. It looks like we don't use TextButton in the emoji autocomplete. What if we followed that example?

Comment on lines +498 to +541
itemBuilder: (context, i) => EmojiPickerListEntry(
pageContext: widget.pageContext,
emoji: _resultsToDisplay[i].candidate,
message: widget.message)))),
Copy link
Collaborator

Choose a reason for hiding this comment

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

The touch feedback is colored by a Material default, overlayColor:

image

I see we don't have a spec for these items' touch feedback in the Figma, but is there something similar there you can infer a good color from?

Also, to me it seems like the touch feedback should extend out to the edge of the bottom action sheet.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, if the user has already reacted with one of the emoji in the results, how about showing that state by marking the item somehow? Maybe similarly (or the same) as the on-press feedback, would that be reasonable?

Comment on lines 437 to 483
void _handleViewModelUpdate() {
if (!mounted) return;
setState(() {
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
void _handleViewModelUpdate() {
if (!mounted) return;
setState(() {
void _handleViewModelUpdate() {
setState(() {

If this element is no longer mounted, dispose should have already been called — see that method's doc. So it should be an invariant that this method only gets called if the element is mounted.

Comment on lines 427 to 430
if (_viewModel != null) {
_controller.removeListener(_handleViewModelUpdate);
_viewModel!.dispose();
_resultsToDisplay = const [];
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 have this part mimic what AutocompleteField does — so if there's an existing query, it gets carried over to the new view-model.

final store = PerAccountStoreWidget.of(context);

if (_viewModel != null) {
_controller.removeListener(_handleViewModelUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

doesn't match addListener

}
_viewModel = EmojiAutocompleteView.init(store: store, query: EmojiAutocompleteQuery(''))
..addListener(_handleViewModelUpdate);
_controller.addListener(_handleControllerUpdate);
Copy link
Member

Choose a reason for hiding this comment

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

This can go in initState (like in AutocompleteField), so the listener gets added only once even if the store gets replaced.

The listener won't be called before this point anyway — it won't be called until there's that TextField attached to the controller, which happens only after this is mounted and built, and onNewStore will get called after the state is mounted and before it's built.

Comment on lines 462 to 463
children: [
Padding(padding: const EdgeInsets.only(left: 8, top: 8),
Copy link
Member

Choose a reason for hiding this comment

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

nit: indent

Suggested change
children: [
Padding(padding: const EdgeInsets.only(left: 8, top: 8),
children: [
Padding(padding: const EdgeInsets.only(left: 8, top: 8),

/// Each emoji must be a unicode emoji.
final List<EmojiCandidate> popularEmojis;

void _onPressed(
Copy link
Member

@gnprice gnprice Dec 7, 2024

Choose a reason for hiding this comment

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

Let's abstract the bulk of this method's implementation out into a function that both this and the emoji picker can call. Probably emoji_reaction.dart is a good home.

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.

Trying out this PR's UI on Friday, I decided ranking the results (#1068) is something we'll need soon too — the lack of ranking was bugging me basically every time I use the emoji picker. So I've just sent #1112 for that.

Happily, that combines perfectly smoothly with the emoji picker: now that this PR uses the emoji autocomplete machinery, it gets the benefit of #1112's ranking as soon as the two branches are merged 🙂

Trying git merge pr/1112 from the tip of this branch, the merge is clean, and the emoji picker works fine and has the new ranking. Running flutter test shows one test that needs to be updated, which I've marked below.

@rajveermalviya It's probably cleanest if for now you don't rebase this PR on top of that one, since each PR has a fair amount of complexity going on already. (Instead just cherry-pick one commit as mentioned below.) But it'd be good to try that git merge and see what changes you have to make to fix that one test, at any point when you're otherwise waiting for the next review; then you can keep that version around so it's ready when the time comes.

..emojiName.equals(emojiName));
}

testWidgets('show, search', (tester) async {
Copy link
Member

Choose a reason for hiding this comment

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

This test will need updates when merging with #1112.

Comment on lines 94 to 102
const zulipPopularEmojis = [
EmojiCandidate(
emojiType: ReactionType.unicodeEmoji,
emojiCode: '1f44d',
emojiName: '+1',
aliases: ['thumbs_up', 'like'],
emojiDisplay: UnicodeEmojiDisplay(
emojiName: '+1',
emojiUnicode: '\u{1f44d}')), // '👍'
Copy link
Member

Choose a reason for hiding this comment

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

I ended up providing this same data as part of #1112. Probably most convenient is if you cherry-pick the relevant commit:
bce5084 emoji: Add list of the "popular" emoji

into this PR.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch 4 times, most recently from a4e2ab9 to a5457c8 Compare December 9, 2024 23:04
child: Row(children: [
Flexible(child: Row(
spacing: 1,
children: List.unmodifiable(EmojiStore.popularEmojiCandidates.mapIndexed((index, emoji) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: pull this callback out as a helper method:

Suggested change
children: List.unmodifiable(EmojiStore.popularEmojiCandidates.mapIndexed((index, emoji) {
children: List.unmodifiable(EmojiStore.popularEmojiCandidates
.mapIndexed(_buildButton)))),

That'll dedent it and I think also help simplify understanding the structure.

size: 24))));
})))),
InkWell(
onTap: _onMorePressed,
Copy link
Member

Choose a reason for hiding this comment

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

nit: "onFoo" for callbacks, "handleFoo" for the method that is called:

Suggested change
onTap: _onMorePressed,
onTap: _handleTapMore,

https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#naming-rules-for-typedefs-and-function-variables

Basically it's useful to have some convention for naming the two sides differently, because lots of places have one of each of these kinds of things in the same scope, both being about the same thing, and so without such a convention you end up with name collisions that have to be resolved by hacks like picking a less-good name for one of the two.

Comment on lines 474 to 477
if (_viewModel != null) {
_viewModel!.dispose();
}
_initViewModel(EmojiAutocompleteQuery(_controller.text));
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
if (_viewModel != null) {
_viewModel!.dispose();
}
_initViewModel(EmojiAutocompleteQuery(_controller.text));
if (_viewModel != null) {
assert(_viewModel!.query == _controller.text);
_viewModel!.dispose();
}
_initViewModel(EmojiAutocompleteQuery(_controller.text));

That should be true, right? And if somehow it's not true, then I think this would have a glitch as it'd change what query the view-model was working on.

Comment on lines 480 to 483
void _handleControllerUpdate() {
if (_viewModel != null) {
_viewModel!.query = EmojiAutocompleteQuery(_controller.text);
}
Copy link
Member

Choose a reason for hiding this comment

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

It should be impossible for _viewModel to be null at this point, right? (Similar reasoning to at #1103 (comment) .) So:

Suggested change
void _handleControllerUpdate() {
if (_viewModel != null) {
_viewModel!.query = EmojiAutocompleteQuery(_controller.text);
}
void _handleControllerUpdate() {
_viewModel!.query = EmojiAutocompleteQuery(_controller.text);

if (_viewModel != null) {
_viewModel!.dispose();
}
_initViewModel(EmojiAutocompleteQuery(_controller.text));
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is the only call site of _initViewModel, so we can simplify by inlining it.

final designVariables = DesignVariables.of(context);

return Column(
mainAxisSize: MainAxisSize.min,
Copy link
Member

Choose a reason for hiding this comment

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

Does this do anything when there's an Expanded child? I'd think it doesn't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah it doesn't, removed.

Comment on lines +573 to +568
doAddOrRemoveReaction(
context: pageContext,
Copy link
Member

Choose a reason for hiding this comment

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

I see, this is why we end up needing to pass pageContext down through all these layers (rather than each widget using its own BuildContext like usual). It's so that if the add/remove request fails, doAddOrRemoveReaction has a (probably-)still-mounted context it can use for showing the error.

I feel like there's probably some better pattern we could use here. But I'm not sure offhand what it'd look like — will probably take a bit of experimenting — and this is the natural extension of the pattern we already use throughout the action sheets. So this pattern is good to stick with for this PR.

Comment on lines +587 to +586
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
final Widget? glyph = switch (emojiDisplay) {
ImageEmojiDisplay() =>
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>
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
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
final Widget? glyph = switch (emojiDisplay) {
ImageEmojiDisplay() =>
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>
// TODO deduplicate this logic with [_EmojiAutocompleteItem]
final emojiDisplay = emoji.emojiDisplay.resolve(store.userSettings);
final Widget? glyph = switch (emojiDisplay) {
ImageEmojiDisplay() =>
ImageEmojiWidget(size: _emojiSize, emojiDisplay: emojiDisplay),
UnicodeEmojiDisplay() =>

And then a similar comment in the other place pointing back to here.

That way at least if we start making changes in one place or the other, we'll have something to remind us that there's another copy it'd be good to keep in sync.

And also a good followup sometime would be to find a way to deduplicate the two.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch from a5457c8 to 8f768ca Compare December 10, 2024 17:34
@rajveermalviya rajveermalviya changed the title reactions: Support adding arbitary reactions Support adding arbitrary reactions Dec 10, 2024
@rajveermalviya
Copy link
Collaborator Author

Thanks for the review @gnprice! Pushed a new revision, PTAL.

@chrisbobbe 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 Dec 10, 2024
@chrisbobbe chrisbobbe assigned gnprice and unassigned chrisbobbe Dec 10, 2024
@chrisbobbe chrisbobbe requested a review from gnprice December 10, 2024 18:24
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!

The implementation code all looks good now.

Switching to another task for the moment, but here's a couple of things I spotted when starting to check against the Figma design. I didn't complete a comparison, so there may be more such things you could find.

(I also haven't yet looked at the tests.)

Comment on lines 606 to 611
child: Row(children: [
if (glyph != null)
Padding(
padding: const EdgeInsets.all(10),
child: glyph),
Flexible(child: Text(label,
Copy link
Member

Choose a reason for hiding this comment

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

The Figma design looks to have 4px spacing between these:
image

Comment on lines +518 to +519
style: const TextStyle(fontSize: 19, height: 26 / 19)))),
TextButton(
Copy link
Member

Choose a reason for hiding this comment

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

The Figma design has 8px between the end of the input and the start of the word "Close":
image

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this does it (and puts the right padding at the end too):

             style: TextButton.styleFrom(
-              padding: EdgeInsets.zero,
+              padding: const EdgeInsets.symmetric(horizontal: 8),

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.

OK, finished comparing with Figma and also reading through the tests. Comments below, in addition to the two above.

This is getting quite close to merge. Looking forward to having it in!

Comment on lines 225 to 452
void _handleTapMore() {
// Dismiss current action sheet before opening emoji picker sheet.
Navigator.of(pageContext).pop();
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
void _handleTapMore() {
// Dismiss current action sheet before opening emoji picker sheet.
Navigator.of(pageContext).pop();
void _handleTapMore() {
// TODO(design): have emoji picker slide in from right and push
// action sheet off to the left
// Dismiss current action sheet before opening emoji picker sheet.
Navigator.of(pageContext).pop();

In the Figma doc Vlad says:

“Other >” is a button, which opens a dialog. At the same time current context menu dialog slides left. While content of the new dialog appears from the right to left.

I don't know if there's an easy off-the-shelf way to get that behavior, so leaving it out is fine for this week's beta; but we can leave that TODO(design) comment to record the discrepancy.

Comment on lines +518 to +519
style: const TextStyle(fontSize: 19, height: 26 / 19)))),
TextButton(
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this does it (and puts the right padding at the end too):

             style: TextButton.styleFrom(
-              padding: EdgeInsets.zero,
+              padding: const EdgeInsets.symmetric(horizontal: 8),

}

return Container(
decoration: BoxDecoration(color: designVariables.contextMenuItemBg.withFadedAlpha(0.12)),
Copy link
Member

Choose a reason for hiding this comment

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

nit: overlong line

Comment on lines 136 to 114
Future<void> tapButton(WidgetTester tester) async {
await tester.tap(find.descendant(
of: find.descendant(
of: find.descendant(
of: find.byType(ReactionButtons),
matching: find.byType(InkWell)),
matching: find.byType(UnicodeEmojiWidget)),
matching: find.text(emojiDisplay.emojiUnicode)));
Copy link
Member

Choose a reason for hiding this comment

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

We can put this more in terms of how the user sees it, and simplify:

Suggested change
Future<void> tapButton(WidgetTester tester) async {
await tester.tap(find.descendant(
of: find.descendant(
of: find.descendant(
of: find.byType(ReactionButtons),
matching: find.byType(InkWell)),
matching: find.byType(UnicodeEmojiWidget)),
matching: find.text(emojiDisplay.emojiUnicode)));
Future<void> tapButton(WidgetTester tester) async {
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text(emojiDisplay.emojiUnicode)));

(I see just find.text alone doesn't work in the "removing" tests, because the same emoji appears in a reaction chip.)

connection.prepare(json: {});
await tapButton(tester);
await tester.pump(Duration.zero);
// Ensure all popular emoji buttons are shown.
Copy link
Member

Choose a reason for hiding this comment

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

This is something we already check with the tests below, right? And they do so in a nice end-to-end way: they check that the actual emoji is shown (with an appropriate Text widget), and that tapping it works. So I don't think we need an extra test checking that.

One thing I guess this test case does is check that we don't have any other options offered here in the same way. But I'd be happy not having a test for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed.

testWidgets('request has an error', (tester) async {
final message = eg.streamMessage();
await setupToMessageActionSheet(tester, message: message, narrow: TopicNarrow.ofMessage(message));
group('emoji picker;', () {
Copy link
Member

Choose a reason for hiding this comment

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

nit: the code this is really testing is in lib/widgets/emoji_reaction.dart, so let's put the test in the corresponding file test/widgets/emoji_reaction_test.dart

(I see it's using the setupToMessageActionSheet helper; but it can import that from this file)

Comment on lines 226 to 228
await tester.tap(find.ancestor(
of: find.byIcon(ZulipIcons.chevron_right),
matching: find.byType(InkWell)));
Copy link
Member

Choose a reason for hiding this comment

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

nit: simplify and put more into user-visible terms:

Suggested change
await tester.tap(find.ancestor(
of: find.byIcon(ZulipIcons.chevron_right),
matching: find.byType(InkWell)));
await tester.tap(find.byIcon(ZulipIcons.chevron_right));

Comment on lines 247 to 248
List<Condition<Object?>> arePopularCandidates = popularCandidates.map((c) =>
conditionEmojiListEntry(
Copy link
Member

Choose a reason for hiding this comment

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

nit: these aren't candidates (i.e. EmojiCandidate), but are entries:

Suggested change
List<Condition<Object?>> arePopularCandidates = popularCandidates.map((c) =>
conditionEmojiListEntry(
List<Condition<Object?>> arePopularEntries = popularCandidates.map((c) =>
conditionEmojiListEntry(

Comment on lines 273 to 274
tester.widget(searchFieldFinder);
await tester.enterText(searchFieldFinder, 'z');
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
tester.widget(searchFieldFinder);
await tester.enterText(searchFieldFinder, 'z');
await tester.enterText(searchFieldFinder, 'z');

Or is there an effect the tester.widget line has which I'm missing?

'result': 'error',
});
await tester.tap(find.text('\u{1f4a4}')); // 'zzz' emoji
await tester.pump(Duration.zero); // error arrives; error dialog shows
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 adapt this test so that it would successfully catch if we accidentally broke in the future the careful work you did to properly pass down pageContext: #1103 (comment)

(As Hixie likes to say: the purpose of a test is to make sure we don't accidentally revert your changes.)

As is, if you apply this diff, no tests fail:

           itemBuilder: (context, i) => EmojiPickerListEntry(
-            pageContext: widget.pageContext,
+            pageContext: context,

You can make the test catch that by doing this:

Suggested change
await tester.pump(Duration.zero); // error arrives; error dialog shows
await tester.pump(); // register tap
await tester.pump(Duration(seconds: 1)); // emoji picker animates away
await tester.pump(Duration(seconds: 1)); // error arrives; error dialog shows

and adding delay: Duration(seconds: 2) to the prepare call.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch 2 times, most recently from 48e5360 to 1beb60d Compare December 11, 2024 06:39
@rajveermalviya
Copy link
Collaborator Author

Thanks for the review @gnprice. Pushed a new revision, PTAL.

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! All looks good now except one question in the tests.

Comment on lines 432 to 434
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('\u{1f4a4}')), warnIfMissed: false); // 'zzz' emoji
Copy link
Member

Choose a reason for hiding this comment

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

Huh, why warnIfMissed false?

Suggested change
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('\u{1f4a4}')), warnIfMissed: false); // 'zzz' emoji
await tester.tap(find.descendant(
of: find.byType(BottomSheet),
matching: find.text('\u{1f4a4}'))); // 'zzz' emoji

Seems like if this tap doesn't actually hit, that'd be a bug somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops; this wasn't meant to be included, it's an itermediate debug leftover.
Fixed now.

@rajveermalviya rajveermalviya force-pushed the pr-emoji-reaction-picker branch from 1beb60d to e8fe3c5 Compare December 11, 2024 20:34
@gnprice gnprice force-pushed the pr-emoji-reaction-picker branch from e8fe3c5 to d9621b8 Compare December 11, 2024 23:00
@gnprice
Copy link
Member

gnprice commented Dec 11, 2024

Thanks! LGTM; rebased and will watch CI, since there were a few conflicts to resolve (in test/widgets/action_sheet_test.dart, icons, and lib/generated/l10n/.)

@gnprice
Copy link
Member

gnprice commented Dec 11, 2024

CI passed, and then I merged #1076 and there's a fresh conflict. That one is trivial, though — so resolved, running tools/check locally to be sure, and then I'll merge.

@gnprice gnprice force-pushed the pr-emoji-reaction-picker branch from d9621b8 to bc1cfb5 Compare December 11, 2024 23:18
@gnprice gnprice merged commit bc1cfb5 into zulip:main Dec 11, 2024
@rajveermalviya rajveermalviya deleted the pr-emoji-reaction-picker branch December 12, 2024 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration review Added by maintainers when PR may be ready for integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support adding an arbitrary reaction
4 participants