Support lightweight "reactions" #541

Closed
cjb opened this Issue Mar 17, 2016 · 23 comments

Projects

None yet

8 participants

@cjb
cjb commented Mar 17, 2016

Emoji responses shown underneath a post.

@tobywhughes

Edit: Just realized github has this feature as well. Never noticed...

Are you saying similar to facebook's new like system?
New system

I personally think that would be an interesting feature to add, but it should definitely be able to be toggled off.

@timabbott
Member

I think this is a feature that we'll likely end up wanting to add at some point in the future, since it's been a popular feature for some similar products. It's probably one of the more complex new features that could be added to Zulip. If anyone's interested in working on this, ping me and I'm happy to help with a technical design.

@Alpus
Contributor
Alpus commented Mar 22, 2016

I think it is interesting for me to make it as part of my summer work. I need more details.

@timabbott
Member

Cool. I think from a UI perspective we should plan to make it something similar to how this works in other products.

From a data storage perspective, probably we'll want to store them on a new MessageReaction table, though we need to be careful about adding additional joins to core flows for performance reasons, so it's possible we'd end up wanting to store all reactions to a message in a field on the Message table. Regardless, it seems like these should be effectively triples (user_id, message_id, reaction). For data flow I think basically it can be passed around in the message dicts, since it's part of what is needed to render a message.

Then for the frontend implementation, we'd need to put them in the appropriate place in the message rendering codepath; probably this will mean adding more code to message_group.handlebars and likely some functions around that to do the rendering.

@timabbott
Member

@jdherg can you post your code for this (doesn't need to be finished) somewhere that other folks could pick it up? I'd like to get this moving again...

@jdherg
Contributor
jdherg commented Aug 26, 2016

Of course! Will do later this weekend. Thanks!

On Aug 26, 2016, at 14:27, Tim Abbott notifications@github.com wrote:

@jdherg can you post your code for this (doesn't need to be finished) somewhere that other folks could pick it up? I'd like to get this moving again...

โ€”
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@cjb
cjb commented Oct 5, 2016

Hi @jdherg! Any update on your WIP branch?

@jdherg
Contributor
jdherg commented Oct 8, 2016

Ah, sorry -- I ended up having a short-notice cross-country move and new job thing right after that last post and haven't had enough time free on my personal laptop (or awake in my own apartment, really) to figure out what's still left or to catch my WIP stuff up with the past couple of months of development. Whoever picks this up can feel free to build from there or if catching it up to master is too much of a mess they can feel free to grab what still makes sense (or nothing at all, if that's how it works out) :)

https://github.com/jdherg/zulip/tree/jdherg-reacji

See you after the election!

@timabbott
Member

Thanks @jdherg, I really appreciate it!

@cjb are you thinking you want to pick up this project, or just interested in the feature?

@cjb
cjb commented Oct 11, 2016

@timabbott I'm super interested in the feature, but not sure how much I can help. I just looked at the branch, it looks like the backend (models and views) are done, but nothing on the frontend side. I'm mostly a backend person, so I'm probably not the right person to work on the UI. I suspect I could respond to review feedback on the backend, if someone else works on the frontend?

@timabbott
Member
timabbott commented Oct 11, 2016 edited

Well, I think a great thing you could do on the backend side is finish the basic implementation and make the tests a bit more extensive. There are several issues with John's backend implementation right now (basically just because it's unfinished):

  • two copies of the MessageReaction class
  • the send_event call sends to the entire realm, not the list of subscribers to the message's recipient object. Something like Subscription.objects.filter(recipient=message.recipient).values("user_profile_id") should work pretty well for this.
  • The do_add_reaction function doesn't handle races quite correctly (we need to catch an IntegrityError on trying to creation the reaction, probably by using something like get_or_create).
  • It's missing the schema migration
  • It doesn't have negative tests for adding invalid emoji, interaction with RealmEmoji, etc.
  • We need to modify get_old_messages to fetch the reaction data in all of its queries (this is probably the largest missing piece and I'd be fine with deferring to a later pull request)
  • We should be using access_message from zerver.lib.message to check whether the user has access to the messages they're trying to react to.

I'm happy to merge a good backend without a frontend (and then find someone excited about the frontend piece, which I think would take <1 week; the hard part is definitely the backend); it won't do anything until there's a frontend, and we can always tweak the backend in future PRs.

So if you can clean up (some of) the backend items listed above and open a pull request for the backend piece, that would be a huge huge help!

@timabbott
Member

(and I'm happy to spend significant time helping with this; feel free to drop by zulip.tabbott.net to be able to collaborate interactively if you want do this!)

@cjb
cjb commented Oct 11, 2016

Thanks for the explanation! I don't want to stop anyone else working on this if they feel like it, but those do sound like things I could help with. I guess I'll mention progress here if I have some, and if you're interested in helping and don't see any more messages from me, it means this is up for grabs.

@brannerchinese

I sure hope this is a feature that is turned off by default, or at worst can be turned off at will. One of the horrors of social media is the amount of metacontent I am forced to look at. Effective workflow means shutting all that stuff out.

@timabbott
Member

We can consider an option to turn it off after we've done the frontend design and maybe iterated a bit based on feedback of how noisy it feels; it's possible that we can come up with a design where ~nobody wants to turn it off. It certainly wouldn't be hard to hide reactions if there was a clear desire for that.

@kracekumar
Contributor

I am interested in working on this feature. I can set aside few days to work on this in next few weeks. I remember @timabbott mentioning; he will be in RC during Thanksgiving weekend. If that's true, I can work on those days.

@timabbott
Member

@kracekumar awesome! We could definitely work on this Monday+Tuesday next week while I'm visiting RC and see how much we can get done. I'd encourage you to prepare by getting your Zulip development environment setup before then, since there won't be a ton of in-person time.

@arpith have you made much progress beyond @jdherg's original version? It looks like https://github.com/zulip/zulip/pull/2245/files is somewhat cleaned up at least...

@arpith
Contributor
arpith commented Nov 15, 2016

@timabbott I expect to have a useable backend by the end of today ๐Ÿ˜ƒ
@kracekumar would love to build this feature with you, shall we pair / work together on the frontend?

@kracekumar
Contributor

@timabbott I will get the development environment up and running by this weekend.

@arpith: I didn't know, you're already into this. I am happy to pair next week and not this week. So please go ahead.

@timabbott timabbott added this to the Zulip roadmap milestone Nov 18, 2016
@timabbott
Member

thanks to @arpith and @kracekumar, the backend for emoji reactions is complete. Now we just need to finish the frontend implementation :).

@timabbott timabbott added a commit that closed this issue Dec 31, 2016
@arpith @timabbott arpith + timabbott Add frontend support for emoji reactions.
This commit replaces the placeholder "clipboard" button with a reaction button.
This is done on any message that can't be edited. Also, on messages sent by
the user the actions popover (toggled by the down chevron icon) contains
an option to add a reaction.

When clicked, a popover with a search bar and a list of emojis is displayed.
If the right sidebar is collapsed (the viewport is small), the popover is placed
to the left of the button.
Focus is set to the search bar. Typing in the search bar filters emojis.

Emojis with which the user has reacted to this message are highlighted.
Clicking them sends an API request to remove that reaction.
Clicking on non-highlighted emojis sends an API request to add a reaction.
When the popover loses focus it is closed.

The frontend listens for reaction events. When an add-reaction event is
received, the emoji is displayed at the bottom of the message with a
count initialized to 1. If there was an existing reaction to the message with
the same emoji, the count is incremented.

Old messages fetched from the server contain reactions.
They are displayed (along with title and count) at the bottom
of each message.

When clicking the emoji reaction at the bottom of the message, if the
user has already reacted with that emoji to this message, the reaction
is removed and the count is decremented. Otherwise, a reaction is added
and the count is incremented.

Hovering over the emoji reaction at the bottom of the message displays
a list of users who have reacted with this emoji along with the
emoji name.

Hovering over the emoji reactions at the bottom of the message displays
a button to add a reaction.

Fixes #541.
9c64a08
@timabbott timabbott closed this in 9c64a08 Dec 31, 2016
@timabbott
Member

We just merged support for this feature into master. Huge thanks to @arpith, @jdherg, @kracekumar, and @stanzheng for their work on this!!

@kracekumar
Contributor

โค๏ธ Rolling out to zulipchat.com will be one of the best new year gift!

@arpith
Contributor
arpith commented Dec 31, 2016

Thanks for merging this @timabbott :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment