Merge branch '16988-use-toggle-for-subscribed-unsubscribed-to-notifications' into 'master'

Resolve "Use toggle for subscribed/unsubscribed to notifications"

Closes #16988

See merge request gitlab-org/gitlab-ce!16408

8 files changed, 71 insertions, 56 deletions

diff --git a/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue b/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue
index 7226076a8fc..d69d100a26c 100644
--- a/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue
+++ b/app/assets/javascripts/sidebar/components/subscriptions/subscriptions.vue
@@ -1,12 +1,22 @@
 <script>
-  /* eslint-disable vue/require-default-prop */
-  import { __ } from '../../../locale';
+  import { __ } from '~/locale';
+  import icon from '~/vue_shared/components/icon.vue';
+  import toggleButton from '~/vue_shared/components/toggle_button.vue';
+  import tooltip from '~/vue_shared/directives/tooltip';
   import eventHub from '../../event_hub';
-  import loadingButton from '../../../vue_shared/components/loading_button.vue';
+
+  const ICON_ON = 'notifications';
+  const ICON_OFF = 'notifications-off';
+  const LABEL_ON = __('Notifications on');
+  const LABEL_OFF = __('Notifications off');
 
   export default {
+    directives: {
+      tooltip,
+    },
     components: {
-      loadingButton,
+      icon,
+      toggleButton,
     },
     props: {
       loading: {
@@ -17,22 +27,23 @@
       subscribed: {
         type: Boolean,
         required: false,
+        default: null,
       },
       id: {
         type: Number,
         required: false,
+        default: null,
       },
     },
     computed: {
-      buttonLabel() {
-        let label;
-        if (this.subscribed === false) {
-          label = __('Subscribe');
-        } else if (this.subscribed === true) {
-          label = __('Unsubscribe');
-        }
-
-        return label;
+      showLoadingState() {
+        return this.subscribed === null;
+      },
+      notificationIcon() {
+        return this.subscribed ? ICON_ON : ICON_OFF;
+      },
+      notificationTooltip() {
+        return this.subscribed ? LABEL_ON : LABEL_OFF;
       },
     },
     methods: {
@@ -46,21 +57,29 @@
 <template>
   <div>
     <div class="sidebar-collapsed-icon">
-      <i
-        class="fa fa-rss"
-        aria-hidden="true"
+      <span
+        v-tooltip
+        :title="notificationTooltip"
+        data-container="body"
+        data-placement="left"
       >
-      </i>
+        <icon
+          :name="notificationIcon"
+          :size="16"
+          aria-hidden="true"
+          class="sidebar-item-icon is-active"
+        />
+      </span>
     </div>
     <span class="issuable-header-text hide-collapsed pull-left">
       {{ __('Notifications') }}
     </span>
-    <loading-button
-      ref="loadingButton"
-      class="btn btn-default pull-right hide-collapsed js-issuable-subscribe-button"
-      :loading="loading"
-      :label="buttonLabel"
-      @click="toggleSubscription"
+    <toggle-button
+      ref="toggleButton"
+      class="pull-right hide-collapsed js-issuable-subscribe-button"
+      :is-loading="showLoadingState"
+      :value="subscribed"
+      @change="toggleSubscription"
     />
   </div>
 </template>
diff --git a/app/assets/javascripts/vue_shared/components/toggle_button.vue b/app/assets/javascripts/vue_shared/components/toggle_button.vue
index 2b12718ae96..09031d3ffa1 100644
--- a/app/assets/javascripts/vue_shared/components/toggle_button.vue
+++ b/app/assets/javascripts/vue_shared/components/toggle_button.vue
@@ -23,11 +23,12 @@
       name: {
         type: String,
         required: false,
-        default: '',
+        default: null,
       },
       value: {
         type: Boolean,
-        required: true,
+        required: false,
+        default: null,
       },
       disabledInput: {
         type: Boolean,
@@ -61,6 +62,7 @@
 <template>
   <label class="toggle-wrapper">
     <input
+      v-if="name"
       type="hidden"
       :name="name"
       :value="value"
diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss
index ae9a8b0182c..759719a72da 100644
--- a/app/assets/stylesheets/pages/issuable.scss
+++ b/app/assets/stylesheets/pages/issuable.scss
@@ -162,10 +162,6 @@
       border: 0;
     }
 
-    span {
-      display: inline-block;
-    }
-
     .select2-container span {
       margin-top: 0;
     }
diff --git a/features/project/issues/issues.feature b/features/project/issues/issues.feature
index d6cfa524a3a..819354bb780 100644
--- a/features/project/issues/issues.feature
+++ b/features/project/issues/issues.feature
@@ -164,7 +164,7 @@ Feature: Project Issues
     Given project "Shop" have "Release 0.4" open issue
     When I visit issue page "Release 0.4"
     Then I should see that I am subscribed
-    When I click button "Unsubscribe"
+    When I click the subscription toggle
     Then I should see that I am unsubscribed
 
   @javascript
diff --git a/features/steps/project/issues/issues.rb b/features/steps/project/issues/issues.rb
index 3843374678c..3cd26bb429b 100644
--- a/features/steps/project/issues/issues.rb
+++ b/features/steps/project/issues/issues.rb
@@ -21,20 +21,20 @@ class Spinach::Features::ProjectIssues < Spinach::FeatureSteps
 
   step 'I should see that I am subscribed' do
     wait_for_requests
-    expect(find('.js-issuable-subscribe-button span')).to have_content 'Unsubscribe'
+    expect(find('.js-issuable-subscribe-button')).to have_css 'button.is-checked'
   end
 
   step 'I should see that I am unsubscribed' do
     wait_for_requests
-    expect(find('.js-issuable-subscribe-button span')).to have_content 'Subscribe'
+    expect(find('.js-issuable-subscribe-button')).to have_css 'button:not(.is-checked)'
   end
 
   step 'I click link "Closed"' do
     find('.issues-state-filters [data-state="closed"] span', text: 'Closed').click
   end
 
-  step 'I click button "Unsubscribe"' do
-    click_on "Unsubscribe"
+  step 'I click the subscription toggle' do
+    find('.js-issuable-subscribe-button button').click
   end
 
   step 'I should see "Release 0.3" in issues' do
diff --git a/spec/features/boards/sidebar_spec.rb b/spec/features/boards/sidebar_spec.rb
index 205900615c4..b2dbfcd0031 100644
--- a/spec/features/boards/sidebar_spec.rb
+++ b/spec/features/boards/sidebar_spec.rb
@@ -334,14 +334,14 @@ describe 'Issue Boards', :js do
       wait_for_requests
 
       page.within('.subscriptions') do
-        click_button 'Subscribe'
+        find('.js-issuable-subscribe-button button:not(.is-checked)').click
         wait_for_requests
 
-        expect(page).to have_content('Unsubscribe')
+        expect(page).to have_css('.js-issuable-subscribe-button button.is-checked')
       end
     end
 
-    it 'has "Unsubscribe" button when already subscribed' do
+    it 'has checked subscription toggle when already subscribed' do
       create(:subscription, user: user, project: project, subscribable: issue2, subscribed: true)
       visit project_board_path(project, board)
       wait_for_requests
@@ -350,10 +350,10 @@ describe 'Issue Boards', :js do
       wait_for_requests
 
       page.within('.subscriptions') do
-        click_button 'Unsubscribe'
+        find('.js-issuable-subscribe-button button.is-checked').click
         wait_for_requests
 
-        expect(page).to have_content('Subscribe')
+        expect(page).to have_css('.js-issuable-subscribe-button button:not(.is-checked)')
       end
     end
   end
diff --git a/spec/features/projects/merge_requests/user_manages_subscription_spec.rb b/spec/features/projects/merge_requests/user_manages_subscription_spec.rb
index 4ca435491cb..f55eb5c6664 100644
--- a/spec/features/projects/merge_requests/user_manages_subscription_spec.rb
+++ b/spec/features/projects/merge_requests/user_manages_subscription_spec.rb
@@ -13,20 +13,18 @@ describe 'User manages subscription', :js do
   end
 
   it 'toggles subscription' do
-    subscribe_button = find('.js-issuable-subscribe-button')
+    page.within('.js-issuable-subscribe-button') do
+      expect(page).to have_css 'button:not(.is-checked)'
+      find('button:not(.is-checked)').click
 
-    expect(subscribe_button).to have_content('Subscribe')
+      wait_for_requests
 
-    click_on('Subscribe')
+      expect(page).to have_css 'button.is-checked'
+      find('button.is-checked').click
 
-    wait_for_requests
+      wait_for_requests
 
-    expect(subscribe_button).to have_content('Unsubscribe')
-
-    click_on('Unsubscribe')
-
-    wait_for_requests
-
-    expect(subscribe_button).to have_content('Subscribe')
+      expect(page).to have_css 'button:not(.is-checked)'
+    end
   end
 end
diff --git a/spec/javascripts/sidebar/subscriptions_spec.js b/spec/javascripts/sidebar/subscriptions_spec.js
index 9b33dd02fb9..79db05f04ed 100644
--- a/spec/javascripts/sidebar/subscriptions_spec.js
+++ b/spec/javascripts/sidebar/subscriptions_spec.js
@@ -20,23 +20,23 @@ describe('Subscriptions', function () {
       subscribed: undefined,
     });
 
-    expect(vm.$refs.loadingButton.loading).toBe(true);
-    expect(vm.$refs.loadingButton.label).toBeUndefined();
+    expect(vm.$refs.toggleButton.isLoading).toBe(true);
+    expect(vm.$refs.toggleButton.$el.querySelector('.project-feature-toggle')).toHaveClass('is-loading');
   });
 
-  it('has "Subscribe" text when currently not subscribed', () => {
+  it('is toggled "off" when currently not subscribed', () => {
     vm = mountComponent(Subscriptions, {
       subscribed: false,
     });
 
-    expect(vm.$refs.loadingButton.label).toBe('Subscribe');
+    expect(vm.$refs.toggleButton.$el.querySelector('.project-feature-toggle')).not.toHaveClass('is-checked');
   });
 
-  it('has "Unsubscribe" text when currently not subscribed', () => {
+  it('is toggled "on" when currently subscribed', () => {
     vm = mountComponent(Subscriptions, {
       subscribed: true,
     });
 
-    expect(vm.$refs.loadingButton.label).toBe('Unsubscribe');
+    expect(vm.$refs.toggleButton.$el.querySelector('.project-feature-toggle')).toHaveClass('is-checked');
   });
 });