webview: Add an "add reaction button" when there are already reactions. #4594

Gautam-Arora24 · 2021-04-03T20:41:14Z

What these changes propose?
Add an "add reaction button" when there is already a reaction in a message. This approach would offer a simple way for a user to add reactions to an already reacted message other than long-pressing a message and then adding a reaction.

How it looks?

Screen-Recording-2021-04-03-at-9.35.18-PM.mov

Fixes: #4578

chrisbobbe

Thanks, @Gautam-Arora24, this is looking good!

Before merging, it'd be great if @gnprice could take another look at whether the plus sign is visible in the add-reaction button in the web app; I've posted a screenshot here where I do see the plus sign along with the smiley face; here it is again:

I think we'd like the visual design to match the web app as exactly as possible (with things like hover behavior, etc., being noted as impossible on mobile). But to do that, we'll need to come to an agreement about what the web app does. 🙂

See a few small comments below.

Also, some commit message nits (mostly, this time, to make things easier for you, actually! 🙂):

feat: Add an "add reaction button" when there are already reactions.

Add an "addReactionButtonAsHtml" in "messagesAsHtml" file which
provides the html to "messageBody" when there is already a
reaction in message.

Create a type in "handleOutboundEvents" called "WebViewOutboundNavigate-
ToEmojiPicker" which helps to navigate to the emoji picker screen.

Add a check in "js.js" to call sendMessage when the target is "add-emoji-
button".

Ran "tools/generate-webview-js".

Discussion of this in CZO [1].

[1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/Add.20Emoji.20Reaction.20Button

Fixes: #4578

I think most of the details here aren't necessary. The first three paragraphs of the body are reciting what happens in the diff. You've made sensible choices for what goes in this commit (all the changes seem to relate to each other), so this information isn't hard for a reviewer or future developer (or archaeologist, etc. 😛) to learn without even looking at the commit message. The commit-message summary, and the Fixes: #4578 line do a good job of saying what gets done in the commit.
The Ran "tools/generate-webview-js". line is unnecessary because our test suite (at least in CI; see Check if generate-webview-js is needed, in tools/test #4582) already requires that tools/generate-webview-js be run when js.js is changed. We don't merge things that don't pass our test suite, so this line is already implied. 🙂
Ah, one nit is, let's not use feat in the summary line's prefix. The prefix should indicate the subsystem or area of code that's affected; something like message-list or msglist or webview would be OK for that.

src/webview/js/js.js

src/webview/html/messageAsHtml.js

chrisbobbe · 2021-04-03T21:51:48Z

src/webview/handleOutboundEvents.js

+    case 'emojiPicker':
+      {
+        const { messageId } = event;
+        NavigationService.dispatch(navigateToEmojiPicker(messageId));
+      }
+      break;
+


Suggested change

case 'emojiPicker':

{

const { messageId } = event;

NavigationService.dispatch(navigateToEmojiPicker(messageId));

}

break;

case 'emojiPicker': {

const { messageId } = event;

NavigationService.dispatch(navigateToEmojiPicker(messageId));

break;

}

Strange; I do see that this follows the curly-bracket placement of some surrounding code, like case 'reaction' and case 'reactionDetails'. But that placement doesn't really make sense to me. Could clean up the surrounding code in an NFC commit if you'd like.

Done :) Will make a separate NFC commit for curly-bracket placement 👍

Well, that curly-bracket placement for case 'reaction' and case 'reactionDetails' was due to the fact that break statement was outside the curly-brackets. Made a separate NFC commit for that :)

src/webview/handleOutboundEvents.js

gnprice · 2021-04-06T00:00:38Z

Before merging, it'd be great if @gnprice could take another look at whether the plus sign is visible in the add-reaction button in the web app;

Just updated the chat thread: the answer is I was looking at a different add-reaction button, namely the one in the top right of a message.

gnprice

Thanks @Gautam-Arora24 ! A few comments, to add to the things Chris mentioned.

src/webview/html/messageAsHtml.js

gnprice · 2021-04-06T00:13:55Z

src/webview/html/messageAsHtml.js

-  return template`<div class="reaction-list">$!${htmlList.join('')}</div>`;
+  return template`<div class="reaction-list">$!${htmlList.join(
+    '',
+  )} $!${addReactionButtonAsHtml()}  </div>`;


nit: there's extra spaces here, as well as in the definition of addReactionButtonAsHtml. In general it's best to avoid having extra spaces or newlines in HTML text -- it's fine inside the tag syntax, after a < before the >, but avoid it in the literal text that comes after a > until the next tag's <. When it's in literal text it's part of what gets displayed, and can lead to hard-to-debug glitches in the layout.

Add an "addReactionButtonAsHtml" in "messagesAsHtml" file which provides the html to "messageBody" when there is already a reaction in message. Discussion of this in CZO [1]. [1] https://chat.zulip.org/#narrow/stream/48-mobile/topic/Add.20Emoji.20Reaction.20Button Fixes: zulip#4578

Gautam-Arora24 · 2021-04-08T10:47:16Z

@gnprice, @chrisbobbe FYI, this PR is ready for a re-review.

chrisbobbe

Thanks, @Gautam-Arora24!

I'd like to see more work aligning the appearance of the button with the web app, or thorough and specific reasoning about why it should be different.

Here are light- and dark-mode screenshots from the web app. (This time I've hovered over the area that makes the button appear, but without hovering over the actual button, which gives its own highlight effect that I think we don't want; that effect was visible in my screenshot above in #4594 (review). Either way, though, the web app is still looking quite different from the mobile app in this PR.)

And corresponding screenshots from this PR:

Here are a few ways this PR differs from the web app. In the PR,

The plus sign is much thicker
The plus sign is not very visible in dark mode
There's no smiley face (Greg mentioned that the thing he'd been looking at without a smiley face was something else: webview: Add an "add reaction button" when there are already reactions. #4594 (comment).)
The button has no outline

Probably the most efficient way to mimic the web app will be to study its html/css and propagate the useful patterns to our code. This may also be productive for discovering ways we might want to diverge from the web app (the lack of hover interaction being the most obvious one that we already know about). I'd like to get closer to a point where we can dissect similarities and differences with the web app more precisely, even at the level of individual lines of code if necessary. Although it's totally unrelated to reactions, rewriteSpoilers shows a close study of what the web app does, and how we choose to depart from it. That was added in #4169; you might find the conversation there helpful.

chrisbobbe · 2021-04-08T14:52:31Z

src/webview/html/messageAsHtml.js

@@ -43,6 +43,8 @@ const messageReactionAsHtml = (
      : codeToEmojiMap[reaction.code]
  }&nbsp;${reaction.count}</span>`;

+const addReactionButtonAsHtml = (): string => template` <span class="add-emoji-button">➕</span>`;


` <span class="add-emoji-button">➕</span>`

There's a space there at the beginning; please remove it or explain why it's there, following Greg's comment about spaces.

chrisbobbe · 2021-04-08T14:57:25Z

src/webview/js/generatedEs3.js

@@ -942,6 +942,13 @@ var compiledWebviewJs = (function (exports) {
      return;
    }

+    if (target.matches('.add-emoji-button')) {


Ah, I didn't catch this in my earlier review, but let's have the add-emoji-button CSS class name align better with the "add-reaction" naming pattern for the WebView outbound event. Could call it .add-reaction, which is similar to the existing .reaction but still conveys the action of adding a new reaction.

Gautam-Arora24 force-pushed the add_reaction_button branch from dacfd9a to 291fe5e Compare April 3, 2021 21:31

chrisbobbe requested changes Apr 3, 2021

View reviewed changes

Gautam-Arora24 force-pushed the add_reaction_button branch 3 times, most recently from 2cc944f to d8ae1f5 Compare April 3, 2021 23:18

Gautam-Arora24 changed the title ~~feat: Add an "add reaction button" when there are already reactions.~~ webview: Add an "add reaction button" when there are already reactions. Apr 3, 2021

Gautam-Arora24 requested a review from chrisbobbe April 4, 2021 12:09

gnprice reviewed Apr 6, 2021

View reviewed changes

Gautam-Arora24 added 2 commits April 6, 2021 15:02

handleOutboundEvents [nfc]: Adjust the curly-bracket position.

28aa65b

Gautam-Arora24 force-pushed the add_reaction_button branch from 3e8aaf5 to 28aa65b Compare April 6, 2021 09:32

Gautam-Arora24 requested a review from gnprice April 6, 2021 09:36

chrisbobbe reviewed Apr 8, 2021

View reviewed changes

gnprice added the a-emoji label Apr 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

webview: Add an "add reaction button" when there are already reactions. #4594

webview: Add an "add reaction button" when there are already reactions. #4594

Gautam-Arora24 commented Apr 3, 2021

chrisbobbe left a comment •

edited

Loading

chrisbobbe Apr 3, 2021

Gautam-Arora24 Apr 3, 2021

Gautam-Arora24 Apr 3, 2021

gnprice commented Apr 6, 2021

gnprice left a comment

gnprice Apr 6, 2021

Gautam-Arora24 commented Apr 8, 2021

chrisbobbe left a comment •

edited

Loading

chrisbobbe Apr 8, 2021

chrisbobbe Apr 8, 2021

webview: Add an "add reaction button" when there are already reactions. #4594

Are you sure you want to change the base?

webview: Add an "add reaction button" when there are already reactions. #4594

Conversation

Gautam-Arora24 commented Apr 3, 2021

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 3, 2021

Choose a reason for hiding this comment

Gautam-Arora24 Apr 3, 2021

Choose a reason for hiding this comment

Gautam-Arora24 Apr 3, 2021

Choose a reason for hiding this comment

gnprice commented Apr 6, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Apr 6, 2021

Choose a reason for hiding this comment

Gautam-Arora24 commented Apr 8, 2021

chrisbobbe left a comment • edited Loading

Choose a reason for hiding this comment

chrisbobbe Apr 8, 2021

Choose a reason for hiding this comment

chrisbobbe Apr 8, 2021

Choose a reason for hiding this comment

chrisbobbe left a comment •

edited

Loading

chrisbobbe left a comment •

edited

Loading