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

Add ContentTypeReaction #402

Closed
wants to merge 2 commits into from
Closed

Conversation

rygine
Copy link
Collaborator

@rygine rygine commented Jun 14, 2023

Proposal: https://github.com/orgs/xmtp/discussions/36
Issue: https://github.com/xmtp-labs/hq/issues/1030

This PR adds the reaction content type with adjustments from the proposal as outlined in the comments of the issue.

TL;DR on the changes from the proposal:

  • emoji => content, to be interpreted by client to allow for custom reactions
  • added action, to allow for removing a reaction

@rygine rygine requested a review from a team as a code owner June 14, 2023 17:06
}

export type ReactionParameters = Pick<Reaction, 'action' | 'reference'> & {
encoding: 'UTF-8'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i hardcoded this type to eliminate the need for an enum. we can add more in the future if necessary.

return {
action: content.parameters.action,
reference: content.parameters.reference,
content: new TextDecoder().decode(content.content),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to put some limit (even a relatively large one like 1kb) on the size of the content? I can't think of a valid use-case for large content

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

base64 encoded SVG, images, etc... could be a thing 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm. For something that different from an emoji, we wouldn't want some other metadata to tell the recipient "decode this like an image".

Is that really an intended use-case of this version of the content type?

Copy link
Collaborator Author

@rygine rygine Jun 14, 2023

Choose a reason for hiding this comment

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

AFAIK for this initial version, we're supporting the standard set of emojis via string tokens, so smile => 😄 (:smile:). this will allow clients to use their own custom emojis like Slack. i don't think the intent of this content type is to store actual content like images, SVGs, etc. so it might make sense to limit the length.

versionMinor: 0,
})

export type Reaction = {
Copy link
Collaborator Author

@rygine rygine Jun 14, 2023

Choose a reason for hiding this comment

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

wonder if we should be namespacing things like this to avoid potential clashes in the future. maybe something like ContentTypeReactionContent or similar?

@neekolas
Copy link
Contributor

Curious why we are doing this here and not in https://github.com/xmtp/xmtp-js-content-types. Seems like having it in a separate repo would make it easier to share with RN

@rygine
Copy link
Collaborator Author

rygine commented Jun 14, 2023

Curious why we are doing this here and not in https://github.com/xmtp/xmtp-js-content-types. Seems like having it in a separate repo would make it easier to share with RN

great call out! this is my first content type. wasn't sure where to put it and i saw the typing notification here, so thought it might be appropriate.

@rygine
Copy link
Collaborator Author

rygine commented Jun 14, 2023

this will be added to a different repo

@rygine rygine closed this Jun 14, 2023
@rygine rygine deleted the rygine/add-reaction-content-type branch June 20, 2023 20:37
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.

None yet

2 participants