Merge branch '229341-update-sort-discussion' into 'master'

Update sort discussion to dropdown

Closes #229302 and #229341

See merge request gitlab-org/gitlab!41201

app/assets/javascripts/notes/components/discussion_filter.vue

 <script>
-import $ from 'jquery';
 import { mapGetters, mapActions } from 'vuex';
-import { GlIcon } from '@gitlab/ui';
+import { GlDropdown, GlDropdownItem, GlDropdownDivider } from '@gitlab/ui';
 import { getLocationHash, doesHashExistInUrl } from '../../lib/utils/url_utility';
 import {
   DISCUSSION_FILTERS_DEFAULT_VALUE,
@@ -14,7 +13,9 @@ import notesEventHub from '../event_hub';
 
 export default {
   components: {
-    GlIcon,
+    GlDropdown,
+    GlDropdownItem,
+    GlDropdownDivider,
   },
   props: {
     filters: {
@@ -66,9 +67,6 @@ export default {
     selectFilter(value, persistFilter = true) {
       const filter = parseInt(value, 10);
 
-      // close dropdown
-      this.toggleDropdown();
-
       if (filter === this.currentValue) return;
       this.currentValue = filter;
       this.filterDiscussion({
@@ -78,9 +76,6 @@ export default {
       });
       this.toggleCommentsForm();
     },
-    toggleDropdown() {
-      $(this.$refs.dropdownToggle).dropdown('toggle');
-    },
     toggleCommentsForm() {
       this.setCommentsDisabled(this.currentValue === HISTORY_ONLY_FILTER_VALUE);
     },
@@ -92,7 +87,6 @@ export default {
 
       if (/^note_/.test(hash) && this.currentValue !== DISCUSSION_FILTERS_DEFAULT_VALUE) {
         this.selectFilter(this.defaultValue, false);
-        this.toggleDropdown(); // close dropdown
         this.setTargetNoteHash(hash);
       }
     },
@@ -109,43 +103,24 @@ export default {
 </script>
 
 <template>
-  <div
+  <gl-dropdown
     v-if="displayFilters"
-    class="discussion-filter-container js-discussion-filter-container d-inline-block align-bottom full-width-mobile"
-  >
-    <button
     id="discussion-filter-dropdown"
-      ref="dropdownToggle"
-      class="btn btn-sm qa-discussion-filter"
-      data-toggle="dropdown"
-      aria-expanded="false"
+    class="gl-mr-3 full-width-mobile discussion-filter-container js-discussion-filter-container qa-discussion-filter"
+    :text="currentFilter.title"
   >
-      {{ currentFilter.title }} <gl-icon name="chevron-down" />
-    </button>
-    <div
-      ref="dropdownMenu"
-      class="dropdown-menu dropdown-menu-selectable dropdown-menu-right"
-      aria-labelledby="discussion-filter-dropdown"
-    >
-      <div class="dropdown-content">
-        <ul>
-          <li
-            v-for="filter in filters"
-            :key="filter.value"
-            :data-filter-type="filterType(filter.value)"
-          >
-            <button
+    <div v-for="filter in filters" :key="filter.value" class="dropdown-item-wrapper">
+      <gl-dropdown-item
+        :is-check-item="true"
+        :is-checked="filter.value === currentValue"
         :class="{ 'is-active': filter.value === currentValue }"
+        :data-filter-type="filterType(filter.value)"
         class="qa-filter-options"
-              type="button"
-              @click="selectFilter(filter.value)"
+        @click.prevent="selectFilter(filter.value)"
       >
         {{ filter.title }}
-            </button>
-            <div v-if="filter.value === defaultValue" class="dropdown-divider"></div>
-          </li>
-        </ul>
-      </div>
-    </div>
+      </gl-dropdown-item>
+      <gl-dropdown-divider v-if="filter.value === defaultValue" />
     </div>
+  </gl-dropdown>
 </template>

app/assets/javascripts/notes/components/sort_discussion.vue

-gs
 <script>
-import { GlIcon } from '@gitlab/ui';
+import { GlDropdown, GlDropdownItem } from '@gitlab/ui';
 import { mapActions, mapGetters } from 'vuex';
 import { __ } from '~/locale';
 import LocalStorageSync from '~/vue_shared/components/local_storage_sync.vue';
@@ -15,7 +14,8 @@ const SORT_OPTIONS = [
 export default {
   SORT_OPTIONS,
   components: {
-    GlIcon,
+    GlDropdown,
+    GlDropdownItem,
     LocalStorageSync,
   },
   mixins: [Tracking.mixin()],
@@ -49,33 +49,27 @@ export default {
 </script>
 
 <template>
-  <div
-    data-testid="sort-discussion-filter"
-    class="gl-mr-2 gl-display-inline-block gl-vertical-align-bottom full-width-mobile"
-  >
+  <div class="gl-mr-3 gl-display-inline-block gl-vertical-align-bottom full-width-mobile">
     <local-storage-sync
       :value="sortDirection"
       :storage-key="storageKey"
       @input="setDiscussionSortDirection"
     />
-    <button class="btn btn-sm js-dropdown-text" data-toggle="dropdown" aria-expanded="false">
-      {{ dropdownText }}
-      <gl-icon name="chevron-down" />
-    </button>
-    <div ref="dropdownMenu" class="dropdown-menu dropdown-menu-selectable dropdown-menu-right">
-      <div class="dropdown-content">
-        <ul>
-          <li v-for="{ text, key, cls } in $options.SORT_OPTIONS" :key="key">
-            <button
-              :class="[cls, { 'is-active': isDropdownItemActive(key) }]"
-              type="button"
+    <gl-dropdown
+      :text="dropdownText"
+      data-testid="sort-discussion-filter"
+      class="js-dropdown-text full-width-mobile"
+    >
+      <gl-dropdown-item
+        v-for="{ text, key, cls } in $options.SORT_OPTIONS"
+        :key="key"
+        :class="cls"
+        :is-check-item="true"
+        :is-checked="isDropdownItemActive(key)"
         @click="fetchSortedDiscussions(key)"
       >
         {{ text }}
-            </button>
-          </li>
-        </ul>
-      </div>
-    </div>
+      </gl-dropdown-item>
+    </gl-dropdown>
   </div>
 </template>

ee/spec/features/epics/epic_show_spec.rb

@@ -133,7 +133,7 @@ RSpec.describe 'Epic show', :js do
 
     it 'shows epic thread filter dropdown' do
       page.within('.js-noteable-awards') do
-        expect(find('.js-discussion-filter-container #discussion-filter-dropdown')).to have_content('Show all activity')
+        expect(find('#discussion-filter-dropdown')).to have_content('Show all activity')
       end
     end
 
@@ -142,9 +142,7 @@ RSpec.describe 'Epic show', :js do
 
       context 'when sorted by `Oldest first`' do
         it 'shows comments in the correct order' do
-          page.within('[data-testid="sort-discussion-filter"]') do
           expect(find('.js-dropdown-text')).to have_content('Oldest first')
-          end
 
           items = all('.timeline-entry .timeline-discussion-body .note-text')
           expect(items[0]).to have_content(notes[0].note)
@@ -155,7 +153,7 @@ RSpec.describe 'Epic show', :js do
       context 'when sorted by `Newest first`' do
         before do
           page.within('[data-testid="sort-discussion-filter"]') do
-            find('button').click
+            find('.js-dropdown-text').click
             find('.js-newest-first').click
             wait_for_requests
           end
@@ -163,7 +161,7 @@ RSpec.describe 'Epic show', :js do
 
         it 'shows comments in the correct order', quarantine: 'https://gitlab.com/gitlab-org/gitlab/-/issues/225637' do
           page.within('[data-testid="sort-discussion-filter"]') do
-            expect(find('.js-dropdown-text')).to have_content('Newest first')
+            expect(find('.js-newest-first')).to have_content('Newest first')
           end
 
           items = all('.timeline-entry .timeline-discussion-body .note-text')

spec/frontend/notes/components/discussion_filter_spec.js

@@ -74,13 +74,15 @@ describe('DiscussionFilter component', () => {
   });
 
   it('renders the all filters', () => {
-    expect(wrapper.findAll('.dropdown-menu li').length).toBe(discussionFiltersMock.length);
+    expect(wrapper.findAll('.discussion-filter-container .dropdown-item').length).toBe(
+      discussionFiltersMock.length,
+    );
   });
 
   it('renders the default selected item', () => {
     expect(
       wrapper
-        .find('#discussion-filter-dropdown')
+        .find('#discussion-filter-dropdown .dropdown-item')
         .text()
         .trim(),
     ).toBe(discussionFiltersMock[0].title);
@@ -88,7 +90,7 @@ describe('DiscussionFilter component', () => {
 
   it('updates to the selected item', () => {
     const filterItem = wrapper.find(
-      `.dropdown-menu li[data-filter-type="${DISCUSSION_FILTER_TYPES.HISTORY}"] button`,
+      `.discussion-filter-container .dropdown-item[data-filter-type="${DISCUSSION_FILTER_TYPES.HISTORY}"]`,
     );
 
     filterItem.trigger('click');
@@ -98,7 +100,9 @@ describe('DiscussionFilter component', () => {
 
   it('only updates when selected filter changes', () => {
     wrapper
-      .find(`.dropdown-menu li[data-filter-type="${DISCUSSION_FILTER_TYPES.ALL}"] button`)
+      .find(
+        `.discussion-filter-container .dropdown-item[data-filter-type="${DISCUSSION_FILTER_TYPES.ALL}"]`,
+      )
       .trigger('click');
 
     expect(filterDiscussion).not.toHaveBeenCalled();
@@ -106,7 +110,7 @@ describe('DiscussionFilter component', () => {
 
   it('disables commenting when "Show history only" filter is applied', () => {
     const filterItem = wrapper.find(
-      `.dropdown-menu li[data-filter-type="${DISCUSSION_FILTER_TYPES.HISTORY}"] button`,
+      `.discussion-filter-container .dropdown-item[data-filter-type="${DISCUSSION_FILTER_TYPES.HISTORY}"]`,
     );
     filterItem.trigger('click');
 
@@ -115,7 +119,7 @@ describe('DiscussionFilter component', () => {
 
   it('enables commenting when "Show history only" filter is not applied', () => {
     const filterItem = wrapper.find(
-      `.dropdown-menu li[data-filter-type="${DISCUSSION_FILTER_TYPES.ALL}"] button`,
+      `.discussion-filter-container .dropdown-item[data-filter-type="${DISCUSSION_FILTER_TYPES.ALL}"]`,
     );
     filterItem.trigger('click');
 
@@ -124,10 +128,10 @@ describe('DiscussionFilter component', () => {
 
   it('renders a dropdown divider for the default filter', () => {
     const defaultFilter = wrapper.findAll(
-      `.dropdown-menu li[data-filter-type="${DISCUSSION_FILTER_TYPES.ALL}"] > *`,
+      `.discussion-filter-container .dropdown-item-wrapper > *`,
     );
 
-    expect(defaultFilter.at(defaultFilter.length - 1).classes('dropdown-divider')).toBe(true);
+    expect(defaultFilter.at(1).classes('gl-new-dropdown-divider')).toBe(true);
   });
 
   describe('Merge request tabs', () => {

spec/frontend/notes/components/sort_discussion_spec.js

@@ -55,7 +55,7 @@ describe('Sort Discussion component', () => {
       it('calls the right actions', () => {
         createComponent();
 
-        wrapper.find('.js-newest-first').trigger('click');
+        wrapper.find('.js-newest-first').vm.$emit('click');
 
         expect(store.dispatch).toHaveBeenCalledWith('setDiscussionSortDirection', DESC);
         expect(Tracking.event).toHaveBeenCalledWith(undefined, 'change_discussion_sort_direction', {
@@ -67,7 +67,7 @@ describe('Sort Discussion component', () => {
     it('shows the "Oldest First" as the dropdown', () => {
       createComponent();
 
-      expect(wrapper.find('.js-dropdown-text').text()).toBe('Oldest first');
+      expect(wrapper.find('.js-dropdown-text').props('text')).toBe('Oldest first');
     });
   });
 
@@ -79,7 +79,7 @@ describe('Sort Discussion component', () => {
 
     describe('when the dropdown item is clicked', () => {
       it('calls the right actions', () => {
-        wrapper.find('.js-oldest-first').trigger('click');
+        wrapper.find('.js-oldest-first').vm.$emit('click');
 
         expect(store.dispatch).toHaveBeenCalledWith('setDiscussionSortDirection', ASC);
         expect(Tracking.event).toHaveBeenCalledWith(undefined, 'change_discussion_sort_direction', {
@@ -87,13 +87,13 @@ describe('Sort Discussion component', () => {
         });
       });
 
-      it('applies the active class to the correct button in the dropdown', () => {
-        expect(wrapper.find('.js-newest-first').classes()).toContain('is-active');
+      it('sets is-checked to true on the active button in the dropdown', () => {
+        expect(wrapper.find('.js-newest-first').props('isChecked')).toBe(true);
       });
     });
 
     it('shows the "Newest First" as the dropdown', () => {
-      expect(wrapper.find('.js-dropdown-text').text()).toBe('Newest first');
+      expect(wrapper.find('.js-dropdown-text').props('text')).toBe('Newest first');
     });
   });
 });