-
-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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 announcements #12662
Add announcements #12662
Conversation
fe89bdf
to
e41ecf4
Compare
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.
In addition to the inline comments, I have a few remarks/questions:
- unlike for TLs and notifications, it seems that if streaming stops working at some point, new announcements will be missed entirely (no polling, nor, unless I'm mistaken, reconnecting to the streaming server?)
- expired announcements seem to not disappear until the web app is restarted; some people typically never restart the web app, which would lead to an ever-growing list
- judging by the code (please add screenshots/screencaps to UI changes), the announcements are displayed just above trending hashtags on single-column, which means it will only be displayed to users using single-column on a sufficiently-large screen. Furthermore, people have complained that trending hashtags were already getting cut off by the window's height, so it might be an issue too.
- It seems to me that whenever an announcement appears, it will offset the selected announcement's index, which is a bit weird: showing neither the announcement that was being displayed, nor the newly-added one (unless the last displayed announcement was the most recent one)
0659828
to
bbe10be
Compare
03357de
to
d1cf11a
Compare
d1cf11a
to
5c6c99a
Compare
Should the view always slide to the newest announcement when it arrives? Or should it always reset to the first one. Or keep showing the one that was previously being shown? |
Would it really be worth it to poll the announcements API at the same rate as home/notifications? |
No, it would definitely not be worth polling at the same rate. Home/notifications are polled pretty often, announcements do not need to be refreshed nearly as often. I'd guess something as low as once every 15 minutes should be fine.
Hm, I'd say either keep to the current announcement and have some way to highlight there is a new one, or, just display the one that got pushed, as this shouldn't happen often anyway. |
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.
Overall this seems ok except for the following facts:
- once discarded, it doesn't seem possible to bring back announcements at all?
- there is no interface or even API to create announcements, the only way is using the rails console…
4550045
to
1808824
Compare
app/javascript/mastodon/features/getting_started/components/announcements.js
Outdated
Show resolved
Hide resolved
app/javascript/mastodon/features/getting_started/components/announcements.js
Outdated
Show resolved
Hide resolved
end | ||
|
||
ActiveRecord::Associations::Preloader.new.preload(records, :custom_emoji) | ||
records |
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.
I'm worried about the performance implications of this…
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.
For sure, but this is a pre-optimization implementation and it might even end up being fine given the low number of announcements.
c1a67ee
to
9212fa2
Compare
9212fa2
to
f6a2c16
Compare
Some of the remarks I have made earlier remain:
EDIT: admin interface to issue announcements added |
eda87a3
to
d84ac53
Compare
d84ac53
to
a8746b3
Compare
a8746b3
to
9546948
Compare
I would rather wait for real user feedback before tackling any of these, since I don't really know what people would expect. I have never wanted to bring a GitHub announcement back to look at it again, so this could just be wasting time unless there's real demand for it. |
fe6d828
to
6e24e99
Compare
When creating announcement, maybe there should be field for duration or something like that, I actually would like to come back to Mastodon after some time and see what changed on my lovely instance and with Mastodon itself (not for all the time all I was off (who'd be happy to see “1/9999…99”?), but at least some part of it), but announcements about planned maintenance have no practical sense for me.
There is no point to bringing back specific announcement, but I see point in having announcements log, like blog, but log (haha, sorry…). Like for example, GitHub has changelog page where you can see all the recent changes and it is very simple, so must be announcements log, just another view (by view I mean… the same thing as if you clicked “Blocked users” menu item) and recent announcements of certain age (define age) or paginated, or infinitely scrollable (?). Link to it can be within the announcements panel, replacing instance domain, like “All announcements” or “View announcements”, or simplified “View all”. Would that work? |
There's This can all be adjusted later |
I do think being able to see dismissed announcements (at least for those that are still active) is needed, because should an announcement have some important or actionable information, users will be forced to either keep it (which does eat some significant screen space), or dismiss it and remember the important information anyway. Furthermore, this is kinda at odds with the reactions feature, since you'd be interested in seeing the reactions, but would also want to dismiss the announcement to reclaim some screen space. EDIT: that being said, this can be addressed with user feedback, so we may want to merge that PR as is for now |
I found some behavior that seemed to be malfunctioning. Please wait a bit for the merge due to questions. |
I like this feature! I applied this PR to the test server and tried it.
The following is future request:
|
- Add `with_dismissed` param to announcements API - Fix end date not being formatted when time range is given - Fix announcement delete causing reactions to send streaming updates - Fix announcements container growing too wide and mascot too small - Fix `all_day` being settable when no time range is given - Change text "Update" to "Announcement"
Fix #11006
This PR introduces the ability for admins to publish announcements visible to local users only. The announcements are distributed through the streaming API (and available through the REST API). They can be scheduled for the future and be bound to a certain time range. Announcements can use the standard syntax for mentions, hashtags, URLs and custom emojis. The announcements, when present, are displayed atop the home feed. Users can add reactions to announcements in the form of unicode and custom emojis, and these reactions are distributed through the streaming API as well.