From 2c2a942fd185170d9df52f408860819d734d0907 Mon Sep 17 00:00:00 2001 From: Jacob Schatz Date: Fri, 29 Apr 2016 16:21:03 -0400 Subject: [PATCH 1/2] Fixed the UI to be more obvious. Refactored the code to not have so many dependencies. Also uses camelCase now. Closes #16988 --- app/assets/javascripts/subscription.js.coffee | 39 ++++++++++++------- app/views/shared/issuable/_sidebar.html.haml | 15 ++++--- 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee index 1a430f3aa474..ffb9df30ddff 100644 --- a/app/assets/javascripts/subscription.js.coffee +++ b/app/assets/javascripts/subscription.js.coffee @@ -2,20 +2,31 @@ class @Subscription constructor: (container) -> $container = $(container) @url = $container.attr('data-url') - @subscribe_button = $container.find('.js-subscribe-button') - @subscription_status = $container.find('.subscription-status') - @subscribe_button.unbind('click').click(@toggleSubscription) + @subscribeButton = $container.find('.js-subscribe-button') + @subscribeButton.off('click').click(@toggleSubscription) + @subscribedHTML = ' Unsubscribe' + @unsubscribedHTML = ' Subscribe' - toggleSubscription: (event) => - btn = $(event.currentTarget) - action = btn.find('span').text() - current_status = @subscription_status.attr('data-status') - btn.addClass('disabled') + toggleSubscription: (e) => + btn = $(e.currentTarget) + subscribed = @subscribeButton.attr('data-subscribed')? + btn + .addClass('disabled') + .prop('disabled','disabled') $.post @url, => - btn.removeClass('disabled') - status = if current_status == 'subscribed' then 'unsubscribed' else 'subscribed' - @subscription_status.attr('data-status', status) - action = if status == 'subscribed' then 'Unsubscribe' else 'Subscribe' - btn.find('span').text(action) - @subscription_status.find('>div').toggleClass('hidden') + subscribed = not subscribed + btn + .removeClass('disabled') + .prop('disabled', false); + if subscribed + @subscribeButton.attr('data-subscribed',true) + btn.html(@subscribedHTML) + btn.closest('div').find('.negation').hide() + return + else + @subscribeButton.removeAttr('data-subscribed', subscribed) + btn.html(@unsubscribedHTML) + btn.closest('div').find('.negation').show() + return + return diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index d6552ae7f189..cbfd4c1d2374 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -147,14 +147,13 @@ = icon('rss') .title.hide-collapsed Notifications - - subscribtion_status = subscribed ? 'subscribed' : 'unsubscribed' - %button.btn.btn-block.btn-gray.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button" } - %span= subscribed ? 'Unsubscribe' : 'Subscribe' - .subscription-status.hide-collapsed{data: {status: subscribtion_status}} - .unsubscribed{class: ( 'hidden' if subscribed )} - You're not receiving notifications from this thread. - .subscribed{class: ( 'hidden' unless subscribed )} - You're receiving notifications because you're subscribed to this thread. + %button.btn.btn-block.btn-gray.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button", data: { subscribed: subscribed } } + = icon(subscribed ? 'volume-up' : 'volume-off') + = subscribed ? 'Unsubscribe' : 'Subscribe' + .subscription-status.hide-collapsed + You are + %span.negation{ style: ("display:none;" if subscribed) } not + subscribed - project_ref = cross_project_reference(@project, issuable) .block.project-reference -- GitLab From 753076e4b5774ef8314385d64fe2fea9387b8e91 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Mon, 6 Jun 2016 11:37:44 +0100 Subject: [PATCH 2/2] Addressed feedback Removed HTML from JS and instead changes the text that is passed from the element --- app/assets/javascripts/subscription.js.coffee | 43 +++++++++++-------- app/views/projects/labels/_label.html.haml | 4 +- app/views/shared/issuable/_sidebar.html.haml | 8 ++-- 3 files changed, 31 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/subscription.js.coffee b/app/assets/javascripts/subscription.js.coffee index ffb9df30ddff..7f8f70cf80f9 100644 --- a/app/assets/javascripts/subscription.js.coffee +++ b/app/assets/javascripts/subscription.js.coffee @@ -3,30 +3,37 @@ class @Subscription $container = $(container) @url = $container.attr('data-url') @subscribeButton = $container.find('.js-subscribe-button') - @subscribeButton.off('click').click(@toggleSubscription) - @subscribedHTML = ' Unsubscribe' - @unsubscribedHTML = ' Subscribe' + @subscribeButton + .off('click') + .on('click', @toggleSubscription) - toggleSubscription: (e) => - btn = $(e.currentTarget) + toggleSubscription: => subscribed = @subscribeButton.attr('data-subscribed')? - btn + @subscribeButton .addClass('disabled') - .prop('disabled','disabled') + .prop('disabled', 'disabled') $.post @url, => subscribed = not subscribed - btn + @subscribeButton .removeClass('disabled') - .prop('disabled', false); + .prop('disabled', false) + .find('.fa') + .toggleClass 'fa-volume-up fa-volume-off' + if subscribed - @subscribeButton.attr('data-subscribed',true) - btn.html(@subscribedHTML) - btn.closest('div').find('.negation').hide() - return + @subscribeButton + .attr('data-subscribed', true) + .find('.subscribe-text') + .text(@subscribeButton.data("unsubscribe-text")) + .closest('.subscription') + .find('.negation') + .hide() else - @subscribeButton.removeAttr('data-subscribed', subscribed) - btn.html(@unsubscribedHTML) - btn.closest('div').find('.negation').show() - return - return + @subscribeButton + .removeAttr('data-subscribed', subscribed) + .find('.subscribe-text') + .text(@subscribeButton.data("subscribe-text")) + .closest('.subscription') + .find('.negation') + .show() diff --git a/app/views/projects/labels/_label.html.haml b/app/views/projects/labels/_label.html.haml index 294fec422c54..befa47ca641e 100644 --- a/app/views/projects/labels/_label.html.haml +++ b/app/views/projects/labels/_label.html.haml @@ -13,8 +13,8 @@ .label-subscription{data: {url: toggle_subscription_namespace_project_label_path(@project.namespace, @project, label)}} .subscription-status{data: {status: label_subscription_status(label)}} - %button.js-subscribe-button.label-subscribe-button.btn.action-buttons{ type: "button", data: { toggle: "tooltip" } } - %span= label_subscription_toggle_button_text(label) + %button.js-subscribe-button.label-subscribe-button.btn.action-buttons{ type: "button", data: { toggle: "tooltip", subscribed: label.subscribed?(current_user), subscribe_text: "Subscribe", unsubscribe_text: "Unsubscribe" } } + %span.subscribe-text= label_subscription_toggle_button_text(label) - if can? current_user, :admin_label, @project = link_to edit_namespace_project_label_path(@project.namespace, @project, label), title: "Edit", class: 'btn action-buttons', data: {toggle: "tooltip"} do diff --git a/app/views/shared/issuable/_sidebar.html.haml b/app/views/shared/issuable/_sidebar.html.haml index cbfd4c1d2374..7bfc22d0f6d8 100644 --- a/app/views/shared/issuable/_sidebar.html.haml +++ b/app/views/shared/issuable/_sidebar.html.haml @@ -147,12 +147,12 @@ = icon('rss') .title.hide-collapsed Notifications - %button.btn.btn-block.btn-gray.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button", data: { subscribed: subscribed } } + %button.btn.btn-block.btn-gray.js-subscribe-button.issuable-subscribe-button.hide-collapsed{ type: "button", data: { subscribed: subscribed, subscribe_text: "Subscribe", unsubscribe_text: "Unsubscribe" } } = icon(subscribed ? 'volume-up' : 'volume-off') - = subscribed ? 'Unsubscribe' : 'Subscribe' + %span.subscribe-text= subscribed ? 'Unsubscribe' : 'Subscribe' .subscription-status.hide-collapsed - You are - %span.negation{ style: ("display:none;" if subscribed) } not + You are + %span.negation{ style: ("display:none;" if subscribed) } not subscribed - project_ref = cross_project_reference(@project, issuable) -- GitLab