Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Glitch-soc/Firefish emoji reactions API #1921
feat: Glitch-soc/Firefish emoji reactions API #1921
Changes from all commits
bafce31
ba691a8
53585e0
703e8d6
553b7c6
4b717d4
4988b11
ac0aa18
d37b65f
2ab80a0
d1c576c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 did you add a new notification type? Is it used?
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.
Yes, here: https://github.com/h3poteto/megalodon/pull/1921/files#diff-727a65323e887a8055dfc36e06244e7622dc446801b15f1027995e8b4c7cf63dR367
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 did you add Reaction even though EmojiReaction exists? What's the difference?
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.
Mostly to stay in line with Firefish's
reaction
notification name. I don't think it's very necessary but helps with differentiating string-only reaction notifications (Pleroma) from Reaction object notificationsThere 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.
Ah, I got it.
You mean
In Pleroma(current):
When we receive an EmojiReaction notification, it contains
emoji
field in the object (it does not containreaction
). And it should beNotificationType.EmojiReaction
.In Firefish:
When we receive a Reaction notification, it contains
reaction
field in the object (it does not containemoji
). And it should beNotificationType.Reaction
.Furthermore, in the future Pleroma, when we receive an EmojiReaction, it contains
reaction
field in the object instead ofemoji
field. And at that time, we can removeNotificationType.EmojiReaction
andemoji
field in the notification object.Correct?
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.
Yep, exactly.