-
-
Notifications
You must be signed in to change notification settings - Fork 801
Feature: Add support for XEP-0444 Message Reactions #3906
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
base: master
Are you sure you want to change the base?
Feature: Add support for XEP-0444 Message Reactions #3906
Conversation
e2289f4 to
cac7f22
Compare
|
@marcellintacite , did you see that it is possible to add «custom emojis» in the ConverseJS emoji picker? (see bellow for an explanation). I think those emojis should not be selectable as a message reaction. With this feature, you can link a code name (for example |
|
I thinks there are 2 other missing things in your implementation: Discovering supportConverseJS must declare to other client that it handles message reactions, by adding the feature The XEP says it MUST be implemented. Also, if you are chatting with a user (in a 1 to 1 conversation) that does not support this feature, maybe we should not display the action in the chatbox. Restricted reactionsThe XEP allow some clients or MUC to limit the set of supported emojis. See https://xmpp.org/extensions/xep-0444.html#disco-restricted So, you should check if there is such limitation (for a user in 1 to 1 conversations, and for the MUC in rooms), and filter the emoji picker. |
|
@marcellintacite: Have you seen @JohnXLivingston comments? |
Yes, i am working on it |
|
@marcellintacite: Good :) |
Alright , thanks. I am sorry i didn't remember. I'll do |
I added a support for them |
can you check my implementation please |
src/plugins/reactions/index.js
Outdated
| */ | ||
| api.listen.on('getMessageActionButtons', (el, buttons) => { | ||
| const is_own_message = el.model.get('sender') === 'me'; | ||
| if (!is_own_message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why removing the buttons on our messages? Nothing says we can't react to our own messages :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcellintacite I tend to agree that you should be able to react to your own messages. At least in Slack you can do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will be working on it
| */ | ||
| shouldBeHidden (shortname) { | ||
| // Helper method for the template which decides whether an | ||
| // emoji should be hidden, based on which skin tone is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment should be updated. It is no more only based on the skin tone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcellintacite Looks like you marked some comments from @JohnXLivingston as resolved without addressing them?
I would add that the (updated) comment should go above the function into the docstring.
What do you mean by "added a support for them"? I can't find the related code. |
Yeah, the XEP compliance seems OK now :) |
Thanks , |
|
Hi @JohnXLivingston , could you please take a look at the MR? I need to finalize it for review now that it's in draft form. |
I took a quick look. Seems good now. I think you can mark it as ready for a review. |
|
@marcellintacite there are 10 failing tests. Looks like a bunch of them are because the CAPS version string is different, since you're now advertising support for a new additional XMPP feature. You'll have to update the tests to check for the new version string. See here for example: The old version string is |
Thanks, i am working on it |
|
Hello @jcbrand , I've updated all the test files to use the new CAPS version string and rebased with the latest changes. I also added the |
|
Thanks @marcellintacite, can you please do a rebase onto the Currently there are a bunch of unrelated commits in this PR. |
…estricted reactions
…estricted reactions
c72d580 to
34ace89
Compare
Done |
jcbrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @marcellintacite. I did a first review from my side and left some review comments.
| registerEvents() { | ||
| this.onKeyDown = (ev) => this.#onKeyDown(ev); | ||
| this.dropdown.addEventListener("hide.bs.dropdown", () => this.onDropdownHide()); | ||
| this.dropdown?.addEventListener("hide.bs.dropdown", () => this.onDropdownHide()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this.dropdown sometimes be undefined?
This seems like a code smell (red flag) that masks a potential underlying problem.
| */ | ||
| shouldBeHidden (shortname) { | ||
| // Helper method for the template which decides whether an | ||
| // emoji should be hidden, based on which skin tone is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcellintacite Looks like you marked some comments from @JohnXLivingston as resolved without addressing them?
I would add that the (updated) comment should go above the function into the docstring.
src/plugins/reactions/index.js
Outdated
| */ | ||
| api.listen.on('getMessageActionButtons', (el, buttons) => { | ||
| const is_own_message = el.model.get('sender') === 'me'; | ||
| if (!is_own_message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marcellintacite I tend to agree that you should be able to react to your own messages. At least in Slack you can do so.
| } | ||
|
|
||
| // Create reaction picker component | ||
| const pickerEl = document.createElement('converse-reaction-picker'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are trying to avoid imperative statements like document.createElement and container.appendChild to update the DOM and instead do everything declaratively via the html tagged template literal from lit-html.
So ideally there should be a <converse-reaction-picker></converse-reaction-picker> component rendered inside a template somewhere.
I understand that the template might be outside of this plugin, which is OK, then that other plugin needs to declare a dependency on this plugin.
If this plugin is not loaded, then the <converse-reaction-picker> element will just not render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I have to render the component directly inside the chatview/templates/chat.js
file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The chatview plugin concerns itself with one-on-one chats, so that's not the best place.
Perhaps it can go into src/shared/chat/templates/message.js ? I think that template is used by both MUCs and 1:1 chats, but you should please double-check.
The picker should render only conditionally, based on a state value, similar to how the OGP metadata is rendered only when ogp_metadata is set.
See here:
| ${el.model.get('ogp_metadata')?.map((m) => { |
cfa8235 to
d033b98
Compare
Feature: Add support for XEP-0444 Message Reactions
Description
This PR implements XEP-0444: Message Reactions, allowing users to react to messages with emojis. It introduces a new plugin (
reactions) that handles the UI for picking reactions, displaying them on messages, and managing the underlying XMPP stanza logic.Key Changes
1. New Plugin:
reactionssrc/plugins/reactions/.<reaction>elements from message stanzas.urn:xmpp:reactions:0).2. UI Components
converse-reaction-picker):3. Technical Details
src/shared/chat/styles/emoji.scss) to ensure consistency between the chat input and the reaction picker.Screenshots / Video
emoji_converse.mp4
This still a Draft
CHANGES.mddocument it in
docs/source/configuration.rstwith
make checkor you can run them in the browser by runningmake serve