On-site notifications#7030
Conversation
pablobm
force-pushed
the
onsite-notifications
branch
from
acdb057 to
6466916
Compare
1ec5
left a comment
1ec5
left a comment
There was a problem hiding this comment.
Yay, I’m excited to use this even before extra polish, instead of missing e-mail notifications that get lost in my inbox!
| <%= t( | ||
| ".notification_header_html", | ||
| :changeset_comment_link => link_to( | ||
| t(".changeset_comment"), | ||
| changeset_path( | ||
| notification.changeset, | ||
| :anchor => "c#{notification.comment_id}" | ||
| ) | ||
| ), | ||
| :commenter_name_link => link_to( | ||
| notification.commenter.display_name, | ||
| notification.commenter | ||
| ), | ||
| :time_ago => friendly_date_ago(notification.timestamp) | ||
| ) %> |
There was a problem hiding this comment.
All other notification types.
As we add more notification types, would there be any opportunity to factor out some common elements to avoid boilerplate? This code to generate the header seems like a candidate for that, except that some notification types probably won’t have anything analogous to a commenter or a page to link to.
Since the intention is to communicate all the information that’s currently going out in e-mails, what if we add a consistent heading that’s based on the subject line of those e-mails? For simplicity, it could remain as plain text. Then, these details specific to changeset comments could be a separate line below it. Or maybe the code to generate a rich text heading could live alongside the existing code that generates the plain text subject line.
There was a problem hiding this comment.
Sharing content with the emails, including the subject line, makes a lot of sense; I'll play a bit with the idea. Sharing code between the two might be tricky but at worst we can accept it as incidental duplication and that's fine.
As for sharing code across notifications, let's see where this goes. This line in particular looks difficult to factor out to me, as each translation line will have different names and fiddly details, but who knows.
|
|
||
| <% @notifications.each do |notification| %> | ||
| <%= render :partial => notification.to_partial_path, :object => notification, :as => :notification %> | ||
| <% end %> |
There was a problem hiding this comment.
This would be a good opportunity to link to the notification preferences in #7001, depending on which PR lands first.
There was a problem hiding this comment.
Good point. Yes, I think users are now used to "click here to change your notification preferences" at places like this.
| </p> | ||
| <blockquote class="mx-2 fst-italic"> | ||
| <p class="text-truncate"><%= notification.comment_body.truncate(200, :separator => " ") %></p> | ||
| </blockquote> |
There was a problem hiding this comment.
Should we add some kind of action button below or to the right of the notification, like an “Open Comment” button that does the same thing as the “Changeset Comment” link currently does? It would be a nice place to put a Mark as Read/Unread button once we have that functionality, similar to what we have in the Inbox.
There was a problem hiding this comment.
Yes, it makes sense to provide a unified "take me to the source of the notification" button.
pablobm
force-pushed
the
onsite-notifications
branch
3 times, most recently
from
c8c1287 to
58acba4
Compare
pablobm
force-pushed
the
onsite-notifications
branch
from
58acba4 to
6cc153f
Compare
pablobm
force-pushed
the
onsite-notifications
branch
from
6cc153f to
72131ba
Compare
2 participants
Partially addresses #7018
This is a partial implementation of on-site notifications, to gather some feedback before I get any deeper into it. At the moment, only one notification type is rendered: changeset comment notifications.
Notifications are listed in a dedicated page so that client apps (eg: JOSM) can easily link to them:
Out of scope for this PR:
What I'm missing in this PR:
Thoughts on this approach? Should I carry on and complete it?