feat: notify on reactions #32475
feat: notify on reactions #32475
Conversation
|
|
Hi @reetp , I'm new to open source contributions and have recently developed a feature based on an issue. I submitted a PR a few days ago. Could you please suggest what my next steps should be to ensure my PR gets merged successfully? |
|
You don't need to ping me. You have the attention of a dev. Keep your PR in line with the latest code and hope they choose to merge it. That will likely be up to the product team. An "Enable/Disable" preference might be worth looking at too - not everyone wants to be bombarded with notifications! |
casalsgh
added
need help
|
@VenkataRohan can you make it availabe as a possible setting? |
|
Hi @casalsgh , sure I am working on it , I will update here as soon as I implement it |
VenkataRohan
force-pushed
the
feat-notify-reactions
branch
from
81b77e1 to
a6d70ed
Compare
ggazzo
left a comment
ggazzo
left a comment
There was a problem hiding this comment.
I think its totally fine doing this with no setting.
what I disagree is to increment the unread, let keep this as ephemeral
VenkataRohan
force-pushed
the
feat-notify-reactions
branch
from
a6d70ed to
01585d5
Compare
|
hey @VenkataRohan thanks for all the changes. lets keep the setting so, its not a big deal. |
| export function parseReaction(reactionText: string, roomtype: RoomType): string { | ||
| let reaction = ''; | ||
| if (roomtype === 'd') { | ||
| reaction = `reacted with ${getEmojiClassNameAndDataTitle(reactionText.trim()).children}`; |
There was a problem hiding this comment.
what if we just use the reaction name and build the message on the clients?
anyway the translation is missing
There was a problem hiding this comment.
Hi @ggazzo ,
I have been working on a translation task to obtain reaction names from the back-end and build messages on the client-side. To achieve this, changes need to be made to one of the following files:
konchatNotifications.ts at line 86, where we receive the notification payload from the back-end.
KonchatNotification.ts at line 36, where the above file calls this function.
However, since useTranslation is a hook and cannot be used in either of these files as they lack React components, I devised an alternative approach. Though it is somewhat verbose, it successfully translates the messages. I will make a commit with this working approach , please take a look.
|
I would like to work on this issue if it is not closed. |
|
Hi @Akshanshkaushal I was waiting for a reply from @casalsgh , @ggazzo , I have already implement the feature and made changes suggested , waiting for the team to review it , and ready to work on any suggestion if required. |
Good job. These sorts of things are like Marmite. Some love them, some hate them. I'd guess it could chew through a lot of push notifications too? This could drive many people mad, especially in busy channels. So options to disable it are good. |
| reactionWithTranslation, | ||
| reacted: reaction !== '', |
There was a problem hiding this comment.
The reactionWithTranslation field is introduced but does not seem to have a default value or fallback mechanism when the translation fails. Ensure that reactionWithTranslation gracefully defaults to the untranslated reaction string to prevent any undefined behavior or empty notifications.
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
Hi @Parvezkhan0, @ggazzo , @casalsgh I have added added fallback mechanism if translation fails as request by @Parvezkhan0 and also some good amount for changes to the code to optimse the translation of "Reacted With" process also I have added the message to which in the notification as shown below. the latest commit a9a4052 has all the change. we can also rebase all the previous commit 
|
6 participants
Proposed changes (including videos or screenshots)
isssue #32297
This PR notifies the user when someone reacts on the message and also increases the unread count in the sidebar
Issue(s)
Steps to test or reproduce
Further comments