Finalize UI for all types of notification settings (#3164)

This commit is contained in:
Ben Halpern 2019-06-14 18:28:38 -04:00 committed by GitHub
parent e15a1adcbc
commit e130400e9e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 205 additions and 107 deletions

View file

@ -898,6 +898,7 @@ article {
flex: 1 0 24px;
max-width: 24px;
margin-left: 5px;
@include themeable(filter, theme-social-icon-invert, invert(0));
}
input {
cursor: pointer;
@ -974,6 +975,9 @@ article {
.dropdown-content {
left: 80px !important;
bottom: 0px !important;
@media screen and (min-width: 1365px) {
bottom: -155px !important;
}
a {
background: transparent !important;
padding-left: 10px !important;
@ -1159,7 +1163,11 @@ article {
right: 0;
left: 0;
z-index: 100;
background: $tan;
@include themeable(
background,
theme-top-bar-background,
$tan
);
font-size: 1em;
text-align: left;
min-width: 300px;
@ -1208,37 +1216,109 @@ article {
}
}
a, label {
a {
font-weight: bold;
color: $black;
@include themeable(
color,
theme-color,
$black
);
padding: 12px 18px;
width: calc(100% - 36px);
display: block;
font-size: 0.8em;
&:hover {
background: $light-gray;
}
@include themeable(
background,
theme-top-bar-background-hover,
$light-gray
);
}
}
label {
width: inherit;
@media screen and (min-width: 400px) {
padding-left: 18px;
font-size: 1em;
}
&.notification-subscriptions-area-row {
padding: 0px;
@include themeable(
border-bottom,
theme-container-border,
1px solid $medium-gray
);
}
.notification-subscriptions-area-header {
padding: 12px 6px;
padding-left: 12px;
font-size: 0.7em;
font-weight: bold;
margin-top: -5px;
@media screen and (min-width: 500px) {
font-size: 0.92em;
border-top-right-radius: 3px;
border-top-left-radius: 3px;
}
@media screen and (min-width: 1365px) {
padding-left: 10px;
font-size: 0.7em;
@include themeable-important(
background,
theme-container-background,
darken($tan, 3%)
);
@include themeable-important(
border-bottom,
theme-subtle-border,
1px solid $light-medium-gray
);
button.notification-subscription-label {
display: inline-block;
width: 90px !important;
margin-left: 5px !important;
text-align: center !important;
float: right !important;
padding: 4px 0px !important;
@include themeable-important(
border,
theme-border,
1px solid $light-medium-gray
);
border-radius: 100px;
&.selected {
visibility: hidden !important;
}
}
}
label.enabled {
cursor: pointer;
}
label.disabled {
pointer-events: none;
button.notification-subscription-label {
width: 100% !important;
opacity: 1.0 !important;
text-align: left;
font-size: 0.8em;
box-sizing: border-box !important;
padding: 13px 4px !important;
padding-left: 13px !important;
margin: auto !important;
cursor: pointer !important;
@include themeable-important(
color,
theme-color,
$black
);
&:hover {
@include themeable-important(
background,
theme-top-bar-background-hover,
$light-gray
);
}
.selected-emoji {
visibility: hidden;
margin-left: 4px;
}
&.selected {
.selected-emoji {
display: inline-block;
visibility: visible;
}
@include themeable-important(
color,
theme-color,
$black
);
}
}
}
}

View file

@ -1,9 +1,8 @@
class NotificationSubscriptionsController < ApplicationController
def show
result = if current_user
NotificationSubscription.exists?(user_id: current_user.id, notifiable_id: params[:notifiable_id], notifiable_type: params[:notifiable_type])
else
false
NotificationSubscription.where(user_id: current_user.id, notifiable_id: params[:notifiable_id], notifiable_type: params[:notifiable_type]).
first&.to_json(only: %i[config]) || { config: "not_subscribed" }
end
respond_to do |format|
format.json { render json: result }
@ -13,15 +12,17 @@ class NotificationSubscriptionsController < ApplicationController
def upsert
not_found unless current_user
@notification_subscription = NotificationSubscription.find_or_initialize_by(user_id: current_user.id, notifiable_id: params[:notifiable_id], notifiable_type: params[:notifiable_type].capitalize, config: "all_comments")
if params[:currently_subscribed] == "true"
@notification_subscription.notifiable.update(receive_notifications: false) if current_user_is_author?
@notification_subscription = NotificationSubscription.find_or_initialize_by(user_id: current_user.id, notifiable_id: params[:notifiable_id], notifiable_type: params[:notifiable_type].capitalize)
if params[:config] == "not_subscribed"
@notification_subscription.delete
elsif params[:currently_subscribed] == "false"
@notification_subscription.notifiable.update(receive_notifications: true) if current_user_is_author?
@notification_subscription.notifiable.update(receive_notifications: false) if current_user_is_author?
else
@notification_subscription.config = params[:config] || "all_comments"
if params[:config] == "all_comments" && current_user_is_author?
@notification_subscription.notifiable.update(receive_notifications: true)
end
@notification_subscription.save!
end
result = @notification_subscription.persisted?
respond_to do |format|
format.json { render json: result }

View file

@ -1,14 +1,14 @@
const label = document.getElementById('notification-subscription-label');
const checkbox = document.getElementById('notification-subcription-checkbox');
const subscriptionStatusInput = document.getElementById(
'notification-subscription-status',
);
const notifiableId = document.getElementById(
'notification-subscription-notifiable-id',
).value;
const notifiableType = document.getElementById(
'notification-subscription-notifiable-type',
).value;
const {notifiableId} = document.getElementById(
'notification-subscriptions-area',
).dataset;
const {notifiableType} = document.getElementById(
'notification-subscriptions-area',
).dataset;
const userStatus = document
.getElementsByTagName('body')[0]
.getAttribute('data-user-status');
@ -24,8 +24,8 @@ if (userStatus === 'logged-in') {
})
.then(response => response.json())
.then(result => {
subscriptionStatusInput.value = result;
checkbox.checked = result;
document.getElementById(`notification-subscription-label_${result.config}`).classList.add('selected');
// checkbox.checked = result;
});
}
@ -38,9 +38,12 @@ if (userStatus === 'logged-out') {
showModal('notification-subscription');
};
} else {
updateStatus = () => {
checkbox.checked = !checkbox.checked;
updateStatus = (target) => {
const allButtons = document.getElementsByClassName('notification-subscription-label');
for(let i = 0; i < allButtons.length; i += 1) {
allButtons[i].classList.remove('selected')
}
target.classList.add('selected');
fetch(`/notification_subscriptions/${notifiableType}/${notifiableId}`, {
method: 'POST',
headers: {
@ -50,37 +53,29 @@ if (userStatus === 'logged-out') {
},
credentials: 'same-origin',
body: JSON.stringify({
currently_subscribed: subscriptionStatusInput.value,
config: target.dataset.payload,
// notifiable params are passed via URL
}),
})
.then(response => response.json())
.then(result => {
subscriptionStatusInput.value = result;
checkbox.checked = result;
label.classList.remove('enabled');
label.classList.add('disabled');
setTimeout(() => {
label.classList.remove('disabled');
label.classList.add('enabled');
}, 1500);
});
};
}
label.addEventListener('click', e => {
e.preventDefault();
updateStatus();
});
checkbox.addEventListener('click', e => {
e.preventDefault();
updateStatus();
});
const subscriptionButtons = document.getElementsByClassName('notification-subscription-label');
checkbox.addEventListener('keydown', e => {
if (e.key === 'Enter') {
updateStatus();
}
});
for(let i = 0; i < subscriptionButtons.length; i += 1) {
subscriptionButtons[i].addEventListener('click', e => {
e.preventDefault();
updateStatus(e.target);
if (typeof sendHapticMessage !== "undefined") {
sendHapticMessage('medium');
}
});
subscriptionButtons[i].addEventListener('keydown', e => {
if (e.key === 'Enter') {
updateStatus(e.target);
}
});
}

View file

@ -3,5 +3,5 @@ class NotificationSubscription < ApplicationRecord
belongs_to :notifiable, polymorphic: true
validates :notifiable_type, inclusion: { in: %w[Comment Article] }
validates :user_id, uniqueness: { scope: %i[notifiable_type notifiable_id] }
validates :config, inclusion: { in: %w[all_comments top_level_comments] }
validates :config, inclusion: { in: %w[all_comments top_level_comments only_author_comments] }
end

View file

@ -16,8 +16,10 @@ module Notifications
comment_user_ids = comment.ancestors.where(receive_notifications: true).pluck(:user_id)
subscribed_user_ids = NotificationSubscription.where(notifiable_id: comment.commentable_id, notifiable_type: "Article", config: "all_comments").pluck(:user_id)
top_level_user_ids = NotificationSubscription.where(notifiable_id: comment.commentable_id, notifiable_type: "Article", config: "top_level_comments").pluck(:user_id) if comment.ancestry.blank?
author_subscriber_user_ids = NotificationSubscription.where(notifiable_id: comment.commentable_id, notifiable_type: "Article", config: "only_author_comments").pluck(:user_id) if comment.user_id == comment.commentable.user_id
user_ids = (comment_user_ids + subscribed_user_ids).to_set
user_ids += top_level_user_ids.to_set if top_level_user_ids
user_ids += author_subscriber_user_ids.to_set if author_subscriber_user_ids
json_data = {
user: user_data(comment.user),
comment: comment_data(comment)
@ -31,7 +33,7 @@ module Notifications
json_data: json_data,
)
# Be careful with this basic first implementation of push notification. Has dependency of Pusher/iPhone sort of tough to test reliably.
send_push_notifications(user_id, "@#{comment.user.username} replied to you:", comment.title, "/notifications/comments") if User.find_by(id: user_id)&.mobile_comment_notifications
send_push_notifications(user_id, "@#{comment.user.username} re: #{comment.parent_or_root_article.title}", comment.title, "/notifications/comments") if User.find_by(id: user_id)&.mobile_comment_notifications
end
return unless comment.commentable.organization_id

View file

@ -30,23 +30,33 @@
</button>
<div class="dropdown-content">
<div>
<% if user_signed_in? %>
<div class="dropdown-link-row notification-subscriptions-area-row"
id="notification-subscriptions-area" data-notifiable-id="<%= @article.id %>" data-notifiable-type="Article">
<div class="notification-subscriptions-area-header">🔔 Notification Subscriptions
<button for="subscribe_notifications" id="notification-subscription-label_not_subscribed" class="notification-subscription-label notification-subscription-label-unsubscribe" data-payload="not_subscribed">
Unsubscribe
</button>
</div>
<button for="subscribe_notifications" id="notification-subscription-label_all_comments" class="notification-subscription-label" data-payload="all_comments">
All Comments <span class="selected-emoji">✅</span>
</button>
<button for="subscribe_notifications" id="notification-subscription-label_top_level_comments" class="notification-subscription-label" data-payload="top_level_comments">
Top Level Comments <span class="selected-emoji">✅</span>
</button>
<button for="subscribe_notifications" id="notification-subscription-label_only_author_comments" class="notification-subscription-label" data-payload="only_author_comments">
Post Author Comments <span class="selected-emoji">✅</span>
</button>
<%= javascript_pack_tag "notificationSubscriptionHandler", defer: true %>
</div>
<% end %>
<div class="dropdown-link-row">
<clipboard-copy for="article-copy-link-input" aria-live="polite" aria-controls="article-copy-link-announcer">
<input type="text" id="article-copy-link-input" value="<%= ApplicationConfig["APP_DOMAIN"] %><%= @article.path %>" readonly />
<input type="text" id="article-copy-link-input" value="<%= ApplicationConfig["APP_PROTOCOL"] %><%= ApplicationConfig["APP_DOMAIN"] %><%= @article.path %>" readonly />
<img id="article-copy-icon" src="<%= asset_path("content-copy.svg") %>" alt="Copy to Clipboard" />
<span id="article-copy-link-announcer" role="alert" hidden>Copied to Clipboard</span>
</clipboard-copy>
</div>
<div class="dropdown-link-row">
<label for="subscribe_notifications" id="notification-subscription-label" class="notification-subscription-label enabled">
Subscribe to Notifications
<input type="hidden" name="subscribe_notifications[currently_subscribed]" id="notification-subscription-status" value="">
<input type="hidden" name="subscribe_notifications[notifiable_id]" id="notification-subscription-notifiable-id" value="<%= @article.id %>">
<input type="hidden" name="subscribe_notifications[notifiable_type]" id="notification-subscription-notifiable-type" value="Article">
<input type="checkbox" name="subscribe_notifications" id="notification-subcription-checkbox" autocomplete="off">
</label>
<%= javascript_pack_tag "notificationSubscriptionHandler", defer: true %>
</div>
<web-share-wrapper shareurl="https://dev.to<%= @article.path %>" sharetext="<%= @article.title %>" template="web-share-button">
<div class="dropdown-link-row">
<a target="_blank" href='https://twitter.com/intent/tweet?text="<%= @article.title %>" by <%= @article.user.twitter_username ? "@" + @article.user.twitter_username : @article.user.name %> %23DEVcommunity https://dev.to<%= @article.path %>'>

View file

@ -1,3 +1,5 @@
<% title "Comment Settings" %>
<style>
.pill {
padding: 8px 20px;
@ -36,14 +38,24 @@
<h1>Comment Settings</h1>
<% if @notification_subscription.persisted? %>
<p>All notifications for this comment and any of your comments in the thread are currently muted.</p>
<% else %>
<h4>Don't want notifications for this thread?</h4>
<p><em>This will mute all notifications for this comment and any of your comments in this chain.</em></p>
<% else %>
<p>All notifications for this comment and any of your comments in the thread are currently muted.</p>
<% end %>
<%= form_for(@notification_subscription, url: "/notification_subscriptions/Comment/#{@comment.id}", method: "post", html: { class: "mute-form" }) do |f| %>
<input type="hidden" name="currently_subscribed" value="<%= @notification_subscription.persisted? %>">
<%= f.submit @notification_subscription.persisted? ? "NOTIFICATIONS MUTED" : "MUTE NOTIFICATIONS", class: "cta pill #{@notification_subscription.persisted? ? 'unmute' : 'mute'}" %>
<input type="hidden" name="config" value="<%= @notification_subscription.persisted? ? "not_subscribed" : "all_comments" %>">
<%= f.submit @notification_subscription.persisted? ? "MUTE NOTIFICATIONS" : "🔕 NOTIFICATIONS MUTED (click to unmute)", class: "cta pill #{@notification_subscription.persisted? ? 'mute' : 'unmute'}" %>
<% end %>
<% if NotificationSubscription.where(user_id: current_user, notifiable_type: "Article", notifiable_id: @comment.commentable_id, config: "all_comments").any? %>
<br/><br/>
<p><em>You are still subscribed to all comments in the broader thread, which will mean you will still receive notifications for replies to this comment.</em></p>
<%= form_for(@notification_subscription, url: "/notification_subscriptions/Article/#{@comment.commentable_id}", method: "post", html: { class: "mute-form" }) do |f| %>
<input type="hidden" name="config" value="not_subscribed">
<%= f.submit "UNSUBSCRIBE FROM PARENT POST", class: "cta pill mute" %>
<% end %>
<% end %>
<br>
</div>

View file

@ -11,12 +11,6 @@
<div class="ellipsis-menu">
<button class="ellipsis-menu-btn"><img src="<%= asset_path("three-dots.svg") %>" class="dropdown-icon" alt="dropdown menu icon"></button>
<ul class="ellipsis-menu hidden">
<li class="ellipsis-menu-item">
<%= form_tag("/notification_subscriptions/Article/#{article.id}", method: :post, data: { type: "json" }) do %>
<input type="hidden" name="currently_subscribed" value="<%= article.receive_notifications %>">
<%= submit_tag article.receive_notifications ? "Mute Notifications" : "Receive Notifications" %>
<% end %>
</li>
<hr />
<li class="ellipsis-menu-item">
<%= form_for(article, method: :patch, remote: true, authenticity_token: true, html: { "data-type": "json" }) do |f| %>
@ -42,10 +36,6 @@
<% end %>
<a href="<%= article.path %>/edit" class="pill cta green">EDIT</a>
<% if manage_view %>
<%= form_tag("/notification_subscriptions/Article/#{article.id}", method: :post, class: "mute-form") do %>
<input type="hidden" name="currently_subscribed" value="<%= article.receive_notifications %>">
<%= submit_tag article.receive_notifications ? "MUTE NOTIFICATIONS" : "NOTIFICATIONS MUTED", class: "cta pill #{article.receive_notifications ? 'mute' : 'unmute'}" %>
<% end %>
<a href="<%= article.path %>/delete_confirm" data-no-instant class="cta pill black">DELETE</a>
<% elsif article.published %>
<a href="<%= article.path %>/manage" class="cta pill black">MANAGE</a>

View file

@ -29,20 +29,20 @@ RSpec.describe "NotificationSubscriptions", type: :request do
it "returns the correct subscription boolean as JSON" do
get "/notification_subscriptions/Article/#{article.id}",
headers: headers
expect(response.body).to eq "true"
expect(JSON.parse(response.body)["config"]).to eq "all_comments"
end
it "returns the correct subscription boolean as JSON if unsubscribed" do
article.notification_subscriptions.first.delete
get "/notification_subscriptions/Article/#{article.id}",
headers: headers
expect(response.body).to eq "false"
expect(JSON.parse(response.body)["config"]).to eq "not_subscribed"
end
end
it "returns a JSON response 'false' if there is no logged in user" do
it "returns a JSON response 'null' if there is no logged in user" do
get "/notification_subscriptions/Article/#{article.id}", headers: headers
expect(response.body).to eq "false"
expect(response.body).to eq "null"
expect(response.content_type).to eq "application/json"
end
end
@ -52,7 +52,7 @@ RSpec.describe "NotificationSubscriptions", type: :request do
expect do
post "/notification_subscriptions/Article/#{article.id}",
headers: headers,
params: { currently_subscribed: "false" }
params: { config: "all_comments" }
end.to raise_error ActiveRecord::RecordNotFound
end
@ -73,7 +73,7 @@ RSpec.describe "NotificationSubscriptions", type: :request do
subscription = article.notification_subscriptions.first
post "/notification_subscriptions/Article/#{article.id}",
headers: headers,
params: { currently_subscribed: "true" }
params: { config: "not_subscribed" }
expect { subscription.reload }.to raise_error ActiveRecord::RecordNotFound
end
@ -81,14 +81,14 @@ RSpec.describe "NotificationSubscriptions", type: :request do
it "updates the article.receive_notifications column correctly if the current_user is the author" do
post "/notification_subscriptions/Article/#{article.id}",
headers: headers,
params: { currently_subscribed: "true" }
params: { config: "not_subscribed" }
expect(article.reload.receive_notifications).to eq false
end
it "updates the comment.receive_notifications column correctly if the current_user is the commenter" do
post "/notification_subscriptions/Comment/#{comment.id}",
headers: headers,
params: { currently_subscribed: "true" }
params: { config: "not_subscribed" }
expect(comment.reload.receive_notifications).to eq false
end
end
@ -101,18 +101,18 @@ RSpec.describe "NotificationSubscriptions", type: :request do
end
it "mutes the parent comment" do
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { currently_subscribed: "true" }
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { config: "not_subscribed" }
expect(parent_comment_by_og.reload.receive_notifications).to be false
end
it "does not mute the someone else's parent comment" do
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { currently_subscribed: "false" }
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { config: "all_comments" }
expect(parent_comment_by_other.reload.receive_notifications).to be true
end
it "unmutes the parent comment if already muted" do
parent_comment_by_og.update(receive_notifications: false)
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { currently_subscribed: "false" }
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { config: "all_comments" }
expect(parent_comment_by_og.reload.receive_notifications).to eq true
end
end
@ -124,7 +124,7 @@ RSpec.describe "NotificationSubscriptions", type: :request do
child2_of_child_of_child_by_og
parent_comment_by_other
sign_in user
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { currently_subscribed: "true" }
post "/notification_subscriptions/Comment/#{parent_comment_by_og.id}", headers: headers, params: { config: "not_subscribed" }
end
it "mutes all of the original commenter's comments in a single thread" do

View file

@ -8,6 +8,7 @@ RSpec.describe Notifications::NewComment::Send, type: :service do
let(:organization) { create(:organization) }
let(:article) { create(:article, :with_notification_subscription, user_id: user.id) }
let(:comment) { create(:comment, commentable: article, user: user2) }
let(:author_comment) { create(:comment, commentable: article, user: user) }
let!(:child_comment) { create(:comment, commentable: article, parent: comment, user: user3) }
it "creates users notifications" do
@ -25,12 +26,19 @@ RSpec.describe Notifications::NewComment::Send, type: :service do
it "creates notifications for all subscribed users" do
create(:notification_subscription, user: user3, notifiable: article)
create(:notification_subscription, user: top_level_subscriber, notifiable: article)
create(:notification_subscription, user: top_level_subscriber, notifiable: article, config: "top_level_comments")
described_class.call(comment)
notified_user_ids = Notification.where(notifiable_type: "Comment", notifiable_id: comment.id).pluck(:user_id)
expect(notified_user_ids.sort).to eq([user.id, user3.id, top_level_subscriber.id].sort)
end
it "creates author comments notification" do
create(:notification_subscription, user: user3, notifiable: article, config: "only_author_comments")
described_class.call(author_comment)
notified_user_ids = Notification.where(notifiable_type: "Comment", notifiable_id: author_comment.reload.id).pluck(:user_id)
expect(notified_user_ids.sort).to eq([user3.id].sort)
end
it "doesn't create a notification for top-level-only subscribed users" do
create(:notification_subscription, user: top_level_subscriber, notifiable: article, config: "top_level_comments")
described_class.call(child_comment)