Merge branch 'simplify-notifications-dropdown' into 'master'

Simplify notifications dropdown

See merge request gitlab-org/gitlab!55522

app/assets/javascripts/notifications/components/custom_notifications_modal.vue

@@ -24,12 +24,21 @@ export default {
       default: '',
     },
   },
+  model: {
+    prop: 'visible',
+    event: 'change',
+  },
   props: {
     modalId: {
       type: String,
       required: false,
       default: 'custom-notifications-modal',
     },
+    visible: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
   data() {
     return {
@@ -95,9 +104,11 @@ export default {
 <template>
   <gl-modal
     ref="modal"
+    :visible="visible"
     :modal-id="modalId"
     :title="$options.i18n.customNotificationsModal.title"
     @show="onOpen"
+    v-on="$listeners"
   >
     <div class="container-fluid">
       <div class="row">

app/assets/javascripts/notifications/components/notifications_dropdown.vue

 <script>
-import {
-  GlButtonGroup,
-  GlButton,
-  GlDropdown,
-  GlDropdownDivider,
-  GlTooltipDirective,
-  GlModalDirective,
-} from '@gitlab/ui';
+import { GlDropdown, GlDropdownDivider, GlTooltipDirective } from '@gitlab/ui';
 import Api from '~/api';
 import { sprintf } from '~/locale';
 import { CUSTOM_LEVEL, i18n } from '../constants';
@@ -16,8 +9,6 @@ import NotificationsDropdownItem from './notifications_dropdown_item.vue';
 export default {
   name: 'NotificationsDropdown',
   components: {
-    GlButtonGroup,
-    GlButton,
     GlDropdown,
     GlDropdownDivider,
     NotificationsDropdownItem,
@@ -25,7 +16,6 @@ export default {
   },
   directives: {
     GlTooltip: GlTooltipDirective,
-    'gl-modal': GlModalDirective,
   },
   inject: {
     containerClass: {
@@ -57,6 +47,7 @@ export default {
     return {
       selectedNotificationLevel: this.initialNotificationLevel,
       isLoading: false,
+      notificationsModalVisible: false,
     };
   },
   computed: {
@@ -95,6 +86,11 @@ export default {
     },
   },
   methods: {
+    openNotificationsModal() {
+      if (this.isCustomNotification) {
+        this.notificationsModalVisible = true;
+      }
+    },
     selectItem(level) {
       if (level !== this.selectedNotificationLevel) {
         this.updateNotificationLevel(level);
@@ -106,10 +102,7 @@ export default {
       try {
         await Api.updateNotificationSettings(this.projectId, this.groupId, { level });
         this.selectedNotificationLevel = level;
-
-        if (level === CUSTOM_LEVEL) {
-          this.$refs.customNotificationsModal.open();
-        }
+        this.openNotificationsModal();
       } catch (error) {
         this.$toast.show(this.$options.i18n.updateNotificationLevelErrorMessage, { type: 'error' });
       } finally {
@@ -125,54 +118,16 @@ export default {
 
 <template>
   <div :class="containerClass">
-    <gl-button-group
-      v-if="isCustomNotification"
-      v-gl-tooltip="{ title: buttonTooltip }"
-      data-testid="notification-button"
-      :class="{ disabled: disabled }"
-      :size="buttonSize"
-    >
-      <gl-button
-        v-gl-modal="$options.modalId"
-        :size="buttonSize"
-        :icon="buttonIcon"
-        :loading="isLoading"
-        :disabled="disabled"
-      >
-        <template v-if="buttonText">{{ buttonText }}</template>
-      </gl-button>
-      <gl-dropdown :size="buttonSize" :disabled="disabled">
-        <notifications-dropdown-item
-          v-for="item in notificationLevels"
-          :key="item.level"
-          :level="item.level"
-          :title="item.title"
-          :description="item.description"
-          :notification-level="selectedNotificationLevel"
-          @item-selected="selectItem"
-        />
-        <gl-dropdown-divider />
-        <notifications-dropdown-item
-          :key="$options.customLevel"
-          :level="$options.customLevel"
-          :title="$options.i18n.notificationTitles.custom"
-          :description="$options.i18n.notificationDescriptions.custom"
-          :notification-level="selectedNotificationLevel"
-          @item-selected="selectItem"
-        />
-      </gl-dropdown>
-    </gl-button-group>
-
     <gl-dropdown
-      v-else
       v-gl-tooltip="{ title: buttonTooltip }"
-      data-testid="notification-button"
-      :text="buttonText"
+      data-testid="notification-dropdown"
+      :size="buttonSize"
       :icon="buttonIcon"
       :loading="isLoading"
-      :size="buttonSize"
       :disabled="disabled"
-      :class="{ disabled: disabled }"
+      :split="isCustomNotification"
+      :text="buttonText"
+      @click="openNotificationsModal"
     >
       <notifications-dropdown-item
         v-for="item in notificationLevels"
@@ -193,6 +148,6 @@ export default {
         @item-selected="selectItem"
       />
     </gl-dropdown>
-    <custom-notifications-modal ref="customNotificationsModal" :modal-id="$options.modalId" />
+    <custom-notifications-modal v-model="notificationsModalVisible" :modal-id="$options.modalId" />
   </div>
 </template>

changelogs/unreleased/simplify-notifications-dropdown.yml

+---
+title: Simplify notifications dropdown
+merge_request: 55522
+author:
+type: changed

spec/features/groups/show_spec.rb

@@ -170,14 +170,14 @@ RSpec.describe 'Group show page' do
     it 'is enabled by default' do
       visit path
 
-      expect(page).to have_selector('[data-testid="notification-button"]:not(.disabled)')
+      expect(page).to have_selector('[data-testid="notification-dropdown"] button:not(.disabled)')
     end
 
     it 'is disabled if emails are disabled' do
       group.update_attribute(:emails_disabled, true)
       visit path
 
-      expect(page).to have_selector('[data-testid="notification-button"].disabled')
+      expect(page).to have_selector('[data-testid="notification-dropdown"] .disabled')
     end
   end
 

spec/features/profiles/user_visits_notifications_tab_spec.rb

@@ -15,17 +15,17 @@ RSpec.describe 'User visits the notifications tab', :js do
   it 'changes the project notifications setting' do
     expect(page).to have_content('Notifications')
 
-    first('[data-testid="notification-button"]').click
+    first('[data-testid="notification-dropdown"]').click
     click_button('On mention')
 
-    expect(page).to have_selector('[data-testid="notification-button"]', text: 'On mention')
+    expect(page).to have_selector('[data-testid="notification-dropdown"]', text: 'On mention')
   end
 
   context 'when project emails are disabled' do
     let(:project) { create(:project, emails_disabled: true) }
 
     it 'notification button is disabled' do
-      expect(page).to have_selector('[data-testid="notification-button"].disabled')
+      expect(page).to have_selector('[data-testid="notification-dropdown"] .disabled')
     end
   end
 end

spec/features/projects/show/user_manages_notifications_spec.rb

@@ -10,7 +10,7 @@ RSpec.describe 'Projects > Show > User manages notifications', :js do
   end
 
   def click_notifications_button
-    first('[data-testid="notification-button"]').click
+    first('[data-testid="notification-dropdown"]').click
   end
 
   it 'changes the notification setting' do
@@ -22,7 +22,7 @@ RSpec.describe 'Projects > Show > User manages notifications', :js do
 
     click_notifications_button
 
-    page.within first('[data-testid="notification-button"]') do
+    page.within first('[data-testid="notification-dropdown"]') do
       expect(page.find('.gl-new-dropdown-item.is-active')).to have_content('On mention')
       expect(page).to have_css('[data-testid="notifications-icon"]')
     end
@@ -33,7 +33,7 @@ RSpec.describe 'Projects > Show > User manages notifications', :js do
     click_notifications_button
     click_button 'Disabled'
 
-    page.within first('[data-testid="notification-button"]') do
+    page.within first('[data-testid="notification-dropdown"]') do
       expect(page).to have_css('[data-testid="notifications-off-icon"]')
     end
   end
@@ -80,7 +80,7 @@ RSpec.describe 'Projects > Show > User manages notifications', :js do
 
     it 'is disabled' do
       visit project_path(project)
-      expect(page).to have_selector('[data-testid="notification-button"].disabled', visible: true)
+      expect(page).to have_selector('[data-testid="notification-dropdown"] .disabled', visible: true)
     end
   end
 end

spec/frontend/notifications/components/notifications_dropdown_spec.js

-import { GlButtonGroup, GlButton, GlDropdown, GlDropdownItem } from '@gitlab/ui';
+import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import axios from 'axios';
 import MockAdapter from 'axios-mock-adapter';
@@ -15,14 +15,10 @@ const mockToastShow = jest.fn();
 describe('NotificationsDropdown', () => {
   let wrapper;
   let mockAxios;
-  let glModalDirective;
 
   function createComponent(injectedProperties = {}) {
-    glModalDirective = jest.fn();
-
     return shallowMount(NotificationsDropdown, {
       stubs: {
-        GlButtonGroup,
         GlDropdown,
         GlDropdownItem,
         NotificationsDropdownItem,
@@ -30,11 +26,6 @@ describe('NotificationsDropdown', () => {
       },
       directives: {
         GlTooltip: createMockDirective(),
-        glModal: {
-          bind(_, { value }) {
-            glModalDirective(value);
-          },
-        },
       },
       provide: {
         dropdownItems: mockDropdownItems,
@@ -49,13 +40,12 @@ describe('NotificationsDropdown', () => {
     });
   }
 
-  const findButtonGroup = () => wrapper.find(GlButtonGroup);
-  const findButton = () => wrapper.find(GlButton);
   const findDropdown = () => wrapper.find(GlDropdown);
   const findByTestId = (testId) => wrapper.find(`[data-testid="${testId}"]`);
   const findAllNotificationsDropdownItems = () => wrapper.findAll(NotificationsDropdownItem);
   const findDropdownItemAt = (index) =>
     findAllNotificationsDropdownItems().at(index).find(GlDropdownItem);
+  const findNotificationsModal = () => wrapper.find(CustomNotificationsModal);
 
   const clickDropdownItemAt = async (index) => {
     const dropdownItem = findDropdownItemAt(index);
@@ -83,8 +73,8 @@ describe('NotificationsDropdown', () => {
         });
       });
 
-      it('renders a button group', () => {
-        expect(findButtonGroup().exists()).toBe(true);
+      it('renders split dropdown', () => {
+        expect(findDropdown().props().split).toBe(true);
       });
 
       it('shows the button text when showLabel is true', () => {
@@ -93,7 +83,7 @@ describe('NotificationsDropdown', () => {
           showLabel: true,
         });
 
-        expect(findButton().text()).toBe('Custom');
+        expect(findDropdown().props().text).toBe('Custom');
       });
 
       it("doesn't show the button text when showLabel is false", () => {
@@ -102,7 +92,7 @@ describe('NotificationsDropdown', () => {
           showLabel: false,
         });
 
-        expect(findButton().text()).toBe('');
+        expect(findDropdown().props().text).toBe(null);
       });
 
       it('opens the modal when the user clicks the button', async () => {
@@ -113,9 +103,9 @@ describe('NotificationsDropdown', () => {
           initialNotificationLevel: 'custom',
         });
 
-        findButton().vm.$emit('click');
+        await findDropdown().vm.$emit('click');
 
-        expect(glModalDirective).toHaveBeenCalled();
+        expect(findNotificationsModal().props().visible).toBe(true);
       });
     });
 
@@ -126,8 +116,8 @@ describe('NotificationsDropdown', () => {
         });
       });
 
-      it('does not render a button group', () => {
-        expect(findButtonGroup().exists()).toBe(false);
+      it('renders unified dropdown', () => {
+        expect(findDropdown().props().split).toBe(false);
       });
 
       it('shows the button text when showLabel is true', () => {
@@ -162,7 +152,7 @@ describe('NotificationsDropdown', () => {
           initialNotificationLevel: level,
         });
 
-        const tooltipElement = findByTestId('notification-button');
+        const tooltipElement = findByTestId('notification-dropdown');
         const tooltip = getBinding(tooltipElement.element, 'gl-tooltip');
 
         expect(tooltip.value.title).toBe(`${tooltipTitlePrefix} - ${title}`);
@@ -264,11 +254,9 @@ describe('NotificationsDropdown', () => {
       mockAxios.onPut('/api/v4/notification_settings').reply(httpStatus.OK, {});
       wrapper = createComponent();
 
-      const mockModalShow = jest.spyOn(wrapper.vm.$refs.customNotificationsModal, 'open');
-
       await clickDropdownItemAt(5);
 
-      expect(mockModalShow).toHaveBeenCalled();
+      expect(findNotificationsModal().props().visible).toBe(true);
     });
   });
 });