Skip to content
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

feat: emoji reactions #45

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

m-doescode
Copy link

@m-doescode m-doescode commented Mar 7, 2025

Adds a button to the hold-tap message menu which opens up a modal to add reactions.
Also adds an add reaction button next to existing reactions under messages.

There is still some work to be done, namely adding reaction buttons in other places (for instance, while previewing images).
I also don't really like the emoji selector package I've chosen, for one because it feels a bit out of place, but additionally because it doesn't work out-of-the-box and requires patching, which loops in an additional dependency (patch-package) which is not ideal.

Also, the picker does not support custom emojis which will be a problem in the future.

Anyway, this is only a draft and sort of request-for-comments from the maintainers of the project. If you have any suggestions or ideas, please feel free to let me know.

I apologize in advance for any bad code styling/formatting. Let me know and I'll clean it up

Future tasklist:

  • Recent emojis instead of "Add reaction" button in message menu sheet
  • Custom emoji support

Please make sure to check the following tasks before opening and submitting a PR

@Rexogamer Rexogamer self-requested a review March 8, 2025 15:03
Copy link
Member

@Rexogamer Rexogamer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the PR! probably about time I stop putting these features off...

some initial thoughts:

  • would you mind splitting the reaction viewer into its own PR? it'll probably be easier to get done/merged than the emoji picker

  • I'm not too sure about how emoji picking is going to be handled - I might add a general emoji picker first before coming back to this?

  • I will note more generally that, while this specific package might not be the right one, for packages that need patching you can/should use yarn patch instead - see .yarn/patches

Comment on lines 74 to 78
onPress={action(() => {
msg.channel?.havePermission('React')
? app.openAddReaction(msg)
: showToast('You cannot react to this message.');
})}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd probably be better to just hide the button if the user doesn't have react perms?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'll wrap it in a ternary checking for permission, then.

<Text>Add Reaction</Text>
</ContextButton>
) : null}
{message.channel?.havePermission('SendMessage') && settings.get('ui.messaging.showReactions') ? (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you added this check in the wrong place

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added what in the wrong place, do you mean the button?

Copy link
Member

@Rexogamer Rexogamer Mar 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the settings check - you're checking whether reactions are enabled for the reply button instead of the reactions button (the settings check should be on line 59)

@m-doescode m-doescode force-pushed the feature/emoji-reactions branch from a43b679 to fb7cb63 Compare March 9, 2025 15:45
@m-doescode
Copy link
Author

Thanks for getting back!

  • would you mind splitting the reaction viewer into its own PR? it'll probably be easier to get done/merged than the emoji picker

Sure thing. Just undid the commits on this PR, will be opening a new one soon.

  • I'm not too sure about how emoji picking is going to be handled - I might add a general emoji picker first before coming back to this?

Probably a good idea. I'm not too pleased with the library I've imported anyway, and it doesn't let you add custom emojis.

  • I will note more generally that, while this specific package might not be the right one, for packages that need patching you can/should use yarn patch instead - see .yarn/patches

Noted. Thanks!

@m-doescode m-doescode force-pushed the feature/emoji-reactions branch 2 times, most recently from 4cf6977 to 65e0613 Compare March 9, 2025 15:56
@m-doescode m-doescode force-pushed the feature/emoji-reactions branch from 65e0613 to 8230945 Compare March 9, 2025 15:56
@m-doescode m-doescode mentioned this pull request Mar 9, 2025
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants