-
Notifications
You must be signed in to change notification settings - Fork 182
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
Emoji reactions support, take 4 #2462
base: main
Are you sure you want to change the base?
Conversation
See: #2221 (comment) Also Font Awesome will likely be removed. |
Wouldn't it be better for UX and general usability to do it "Misskey-Style" and show the usernames on hover? The list could be added anyway for a better mass-overview. |
If you do it MissKey-style (hover on mouse-over), how would you do that on mobile devices, where there is no mouse? |
Misskey and derivatives use both a mouse-over for a quick peek (shows only 11 people and then a "+[n]") and have either a link in the post's menu or a tab to list again all reactions and who used it, similarly to how Discord does it. Edit: Adding an icon + a total count next to the favorites and boosts, like the first pic from Essem, is a good idea and keeps it consistent with the other interactions. |
I'm still for removing modifications of the vanilla flavour. Also this modifies locales managed by Crowdin again |
Ah, apologies; I didn't realize that's why they were removed. Fixed. |
app/mailers/notification_mailer.rb
Outdated
@@ -6,8 +6,8 @@ class NotificationMailer < ApplicationMailer | |||
:routing | |||
|
|||
before_action :process_params | |||
before_action :set_status, only: [:mention, :favourite, :reblog] | |||
before_action :set_account, only: [:follow, :favourite, :reblog, :follow_request] | |||
before_action :set_status, only: [:mention, :favourite, :reaction, :reblog] |
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.
From a server owner cost management and email reputation perspective, sending an email per reaction could get incredibly expensive really fast.
It would be great to have an option for
- roll reactions these up into 30-60min aggregate mails
- a server setting to outright disable email notifications for reactions
Personally, I would use (2) on my server, simply because the cost aspect of potentially sending 100s of mails to a user is not something I'm interested in bankrolling on behalf of my users, and it can have significant email deliverability impact as well if users get spammed and never opens/interacts with them.
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.
Reaction email notifications are disabled by default in user settings and notification emails of any type are only sent to inactive users anyways (unless a specific setting is manually enabled).
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.
It's great its disabled by default, but I do think it would be great to have a server setting to turn it off, with no way to turn it on - I really don't want to see someone (myself included) being burned by potentially expensive transactional mails due to malice of some trolls, or some random toot going viral and burning through mail credits.
It can be quite expensive for on-demand transactional emails, and can prohibit verification mails from going out if the server admin is using a pre-paid delivery plan with X mails per month.
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.
While the concern is understandable, this is not something this PR can or even should address.
As already stated, Mastodon rarely even sends e-mail notifications by default and most people don't enable those.
This just adds the option to receive e-mails for reactions, in line with other notifications.
Concerns about potentially burning e-mail credit should be brought up upstream, but this is pretty much a non-issue in practice.
This pull request has merge conflicts that must be resolved before it can be merged. |
58a54da
to
1b0b4e9
Compare
This pull request has resolved merge conflicts and is ready for review. |
1b0b4e9
to
4d0e0e6
Compare
@TheEssem what're the blockers for un-drafting this, atm, btw? |
This pull request has merge conflicts that must be resolved before it can be merged. |
I'm mainly just waiting for further review. |
If you mean maintainer review then I'd suggest making it non-draft, since else (fme) maintainers won't look at it |
I'm sorry I really don't have much mental bandwidth right now, and my priority is bringing glitch-soc to a more maintainable state. There's a lot I've postponed, and it's becoming a lot. |
4d0e0e6
to
39ef8ca
Compare
This pull request has resolved merge conflicts and is ready for review. |
Is this suitable for being merged into a fork? |
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.
LGTM
Running in production at |
This was in a previous PR. Not quite sure how it didn't carry over.
Probably worth tackling later, but for now it's not worth worrying about; some other implementations (e.g. Misskey's) look to have the same behavior anyways.
Reaction icon made by [email protected]
This was done to announcement reactions in 1b0cb3b. Might as well do it here too.
…r emoji reaction code
This prevents things from breaking with embeds.
2cf6393
to
e58855d
Compare
This pull request has resolved merge conflicts and is ready for review. |
Replaces #1980, #2127, and #2221.
All previous commits have been merged and rebased. I've also fixed a couple of formatting issues and inconsistencies.
Let me know if there's anything missing. I hope to have this PR serve as a base for continued development of this patch.