Skip to content

Show message reactions #121

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

Closed
gnprice opened this issue May 26, 2023 · 2 comments · Fixed by #410
Closed

Show message reactions #121

gnprice opened this issue May 26, 2023 · 2 comments · Fixed by #410
Assignees
Labels
a-api Implementing specific parts of the Zulip server API a-msglist The message-list screen, except what's label:a-content
Milestone

Comments

@gnprice
Copy link
Member

gnprice commented May 26, 2023

This should include handling events that add and remove reactions.

@gnprice gnprice added a-msglist The message-list screen, except what's label:a-content a-api Implementing specific parts of the Zulip server API labels May 26, 2023
@gnprice gnprice added this to the Alpha milestone May 27, 2023
@gnprice gnprice removed the m-alpha label May 27, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 3, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 3, 2023
Ongoing discussion about how to handle these:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20reaction.20events/near/1619282

But for now, at least we can represent the events in the API model,
before we handle them.

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 3, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 3, 2023
Ongoing discussion about how to handle these:
  https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/flutter.3A.20reaction.20events/near/1619282

But for now, at least we can represent the events in the API model,
before we handle them.

Related: zulip#121
@chrisbobbe chrisbobbe self-assigned this Aug 3, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 4, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 4, 2023
We don't yet have UI to show the events (zulip#121), but now at least
we're keeping our Message objects up-to-date with reactions.

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 7, 2023
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 7, 2023
We don't yet have UI to show the events (zulip#121), but now at least
we're keeping our Message objects up-to-date with reactions.

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 8, 2023
We don't yet have UI to show the events (zulip#121), but now at least
we're keeping our Message objects up-to-date with reactions.

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 8, 2023
We don't yet have UI to show the events (zulip#121), but now at least
we're keeping our Message objects up-to-date with reactions.

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 9, 2023
This will make [displayEmojiReactionUsers] available for zulip#121,
showing message reactions. (Though it won't be live-updated yet;
we'll do that soon.)

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 10, 2023
This will make [displayEmojiReactionUsers] available for zulip#121,
showing message reactions. (Though it won't be live-updated yet;
we'll do that soon.)

Related: zulip#121
@chrisbobbe
Copy link
Collaborator

chrisbobbe commented Aug 10, 2023

From studying the web app's layout and the mobile app's current widget tree, I think the right place in that tree for a "reactions" row would be here (marked // <-- here):

Code
/// A Zulip message, showing the sender's name and avatar.
class MessageWithSender extends StatelessWidget {
  const MessageWithSender(
    {super.key, required this.message, required this.content});

  final Message message;
  final ZulipContent content;

  @override
  Widget build(BuildContext context) {
    final time = _kMessageTimestampFormat
      .format(DateTime.fromMillisecondsSinceEpoch(1000 * message.timestamp));

    return GestureDetector(
      behavior: HitTestBehavior.translucent,
      onLongPress: () => showMessageActionSheet(context: context, message: message),
      // TODO clean up this layout, by less precisely imitating web
      child: Padding(
        padding: const EdgeInsets.only(top: 2, bottom: 3, left: 8, right: 8),
        child: Row(crossAxisAlignment: CrossAxisAlignment.start, children: [
          Padding(
            padding: const EdgeInsets.fromLTRB(3, 6, 11, 0),
            child: Avatar(userId: message.senderId, size: 35, borderRadius: 4)),
          Expanded(
            child: Column(
              crossAxisAlignment: CrossAxisAlignment.stretch,
              children: [
                const SizedBox(height: 3),
                Text(message.senderFullName, // TODO get from user data
                  style: const TextStyle(fontWeight: FontWeight.bold)),
                const SizedBox(height: 4),
                MessageContent(message: message, content: content),
                // <-- here
              ])),
          Container(
            width: 80,
            padding: const EdgeInsets.only(top: 4, right: 2),
            alignment: Alignment.topRight,
            child: Text(time, style: _kMessageTimestampStyle)),
        ])));
  }
}
Screenshot

Greg, are you aware of any problems with putting it there, or conflicts with your work on #174 / #175?

@gnprice
Copy link
Member Author

gnprice commented Aug 11, 2023

Sounds good to me!

There won't be any substantive conflict with my branch for #174 / #175. Might be a small textual conflict from touching nearby lines of code, but it'll be easy to resolve.

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 11, 2023
This will make [displayEmojiReactionUsers] available for zulip#121,
showing message reactions. (Though it won't be live-updated yet;
we'll do that soon.)

Related: zulip#121
gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this issue Aug 14, 2023
This will make [displayEmojiReactionUsers] available for zulip#121,
showing message reactions. (Though it won't be live-updated yet;
we'll do that soon.)

Related: zulip#121
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 22, 2023
And also support:
- removing a reaction you've already made, and
- joining in on existing reactions that other people have made.

It leaves out one part of zulip#125, for now:
- joining in on an existing reaction other people have made

As is our habit with the message list, this aims to be faithful to
the web app, as accessed today. That should be a good baseline to
make mobile-specific adjustments from. (In particular I think we'll
want larger touch targets.)

Unlike the web app, we use a font instead of a sprite sheet to
render Unicode emoji. This means that we, unlike web, have to
account for text-layout algorithms, and things like font metrics. So
if Unicode emoji appear noticeably differently from web, that's
worth being aware of.

Fixes: zulip#121
Fixes-partly: zulip#125
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 27, 2023
And also support:
- removing a reaction you've already made, and
- joining in on existing reactions that other people have made.

As is our habit with the message list, this aims to be faithful to
the web app, as accessed today. That should be a good baseline to
make mobile-specific adjustments from. (In particular I think we'll
want larger touch targets.)

Unlike the web app, we use a font instead of a sprite sheet to
render Unicode emoji. This means that we, unlike web, have to
account for text-layout algorithms, and things like font metrics. So
if Unicode emoji appear noticeably differently from web, that's
worth being aware of.

Fixes: zulip#121
Fixes-partly: zulip#125
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 30, 2023
And also support:
- removing a reaction you've already made, and
- joining in on existing reactions that other people have made.

As is our habit with the message list, this aims to be faithful to
the web app, as accessed today. That should be a good baseline to
make mobile-specific adjustments from. (In particular I think we may
want larger touch targets.)

Unlike the web app, we use a font instead of a sprite sheet to
render Unicode emoji. This means that we, unlike web, have to
account for text-layout algorithms, and things like font metrics. So
if Unicode emoji appear noticeably differently from web, that
probably has something to do with it.

Fixes: zulip#121
Fixes-partly: zulip#125
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Nov 30, 2023
And also support:
- removing a reaction you've already made, and
- joining in on existing reactions that other people have made.

As is our habit with the message list, this aims to be faithful to
the web app, as accessed today. That should be a good baseline to
make mobile-specific adjustments from. (In particular I think we may
want larger touch targets.)

Unlike the web app, we use a font instead of a sprite sheet to
render Unicode emoji. This means that we, unlike web, have to
account for text-layout algorithms, and things like font metrics. So
if Unicode emoji appear noticeably differently from web, that
probably has something to do with it.

Fixes: zulip#121
Fixes-partly: zulip#125
chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this issue Dec 1, 2023
And also support:
- removing a reaction you've already made, and
- joining in on existing reactions that other people have made.

As is our habit with the message list, this aims to be faithful to
the web app, as accessed today. That should be a good baseline to
make mobile-specific adjustments from. (In particular I think we may
want larger touch targets.)

Unlike the web app, we use a font instead of a sprite sheet to
render Unicode emoji. This means that we, unlike web, have to
account for text-layout algorithms, and things like font metrics. So
if Unicode emoji appear noticeably differently from web, that
probably has something to do with it.

Fixes: zulip#121
Fixes-partly: zulip#125
@gnprice gnprice closed this as completed in 6337a98 Dec 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-api Implementing specific parts of the Zulip server API a-msglist The message-list screen, except what's label:a-content
Projects
Status: Done
2 participants