Merge branch '228909-improve-file-by-file-diffs-prev-next-pagination' into 'master'

Improve file-by-file navigation

See merge request gitlab-org/gitlab!39719

app/assets/javascripts/diffs/components/app.vue

 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
-import { GlLoadingIcon, GlButtonGroup, GlButton, GlAlert } from '@gitlab/ui';
+import { GlLoadingIcon, GlButton, GlAlert, GlPagination, GlSprintf } from '@gitlab/ui';
 import Mousetrap from 'mousetrap';
 import { __ } from '~/locale';
 import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
@@ -37,9 +37,10 @@ export default {
     TreeList,
     GlLoadingIcon,
     PanelResizer,
-    GlButtonGroup,
+    GlPagination,
     GlButton,
     GlAlert,
+    GlSprintf,
   },
   mixins: [glFeatureFlagsMixin()],
   props: {
@@ -169,6 +170,22 @@ export default {
     isDiffHead() {
       return parseBoolean(getParameterByName('diff_head'));
     },
+    showFileByFileNavigation() {
+      return this.diffFiles.length > 1 && this.viewDiffsFileByFile;
+    },
+    currentFileNumber() {
+      return this.currentDiffIndex + 1;
+    },
+    previousFileNumber() {
+      const { currentDiffIndex } = this;
+
+      return currentDiffIndex >= 1 ? currentDiffIndex : null;
+    },
+    nextFileNumber() {
+      const { currentFileNumber, diffFiles } = this;
+
+      return currentFileNumber < diffFiles.length ? currentFileNumber + 1 : null;
+    },
   },
   watch: {
     commit(newCommit, oldCommit) {
@@ -274,6 +291,9 @@ export default {
       'toggleShowTreeList',
       'navigateToDiffFileIndex',
     ]),
+    navigateToDiffFileNumber(number) {
+      this.navigateToDiffFileIndex(number - 1);
+    },
     refetchDiffData() {
       this.fetchData(false);
     },
@@ -509,23 +529,22 @@ export default {
               :can-current-user-fork="canCurrentUserFork"
               :view-diffs-file-by-file="viewDiffsFileByFile"
             />
-            <div v-if="viewDiffsFileByFile" class="d-flex gl-justify-content-center">
-              <gl-button-group>
-                <gl-button
-                  :disabled="currentDiffIndex === 0"
-                  data-testid="singleFilePrevious"
-                  @click="navigateToDiffFileIndex(currentDiffIndex - 1)"
-                >
-                  {{ __('Prev') }}
-                </gl-button>
-                <gl-button
-                  :disabled="currentDiffIndex === diffFiles.length - 1"
-                  data-testid="singleFileNext"
-                  @click="navigateToDiffFileIndex(currentDiffIndex + 1)"
-                >
-                  {{ __('Next') }}
-                </gl-button>
-              </gl-button-group>
+            <div
+              v-if="showFileByFileNavigation"
+              data-testid="file-by-file-navigation"
+              class="gl-display-grid gl-text-center"
+            >
+              <gl-pagination
+                class="gl-mx-auto"
+                :value="currentFileNumber"
+                :prev-page="previousFileNumber"
+                :next-page="nextFileNumber"
+                @input="navigateToDiffFileNumber"
+              />
+              <gl-sprintf :message="__('File %{current} of %{total}')">
+                <template #current>{{ currentFileNumber }}</template>
+                <template #total>{{ diffFiles.length }}</template>
+              </gl-sprintf>
             </div>
           </template>
           <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" />

changelogs/unreleased/maintenance-improve-file-by-file.yml

+---
+title: Tweak file-by-file display and add file current/total display
+merge_request: 39719
+author:
+type: changed

locale/gitlab.pot

@@ -10660,6 +10660,9 @@ msgstr ""
 msgid "File"
 msgstr ""
 
+msgid "File %{current} of %{total}"
+msgstr ""
+
 msgid "File Hooks"
 msgstr ""
 

spec/features/merge_request/user_views_auto_expanding_diff_spec.rb

@@ -27,9 +27,10 @@ RSpec.describe 'User views diffs file-by-file', :js do
     page.within('#diffs') do
       expect(page).not_to have_content('This diff is collapsed')
 
-      click_button 'Next'
+      find('.page-link.next-page-item').click
 
       expect(page).not_to have_content('This diff is collapsed')
+      expect(page).to have_selector('.diff-file .file-title', text: 'large_diff_renamed.md')
     end
   end
 end

spec/features/merge_request/user_views_diffs_file_by_file_spec.rb

@@ -25,7 +25,7 @@ RSpec.describe 'User views diffs file-by-file', :js do
       expect(page).to have_selector('.file-holder', count: 1)
       expect(page).to have_selector('.diff-file .file-title', text: '.DS_Store')
 
-      click_button 'Next'
+      find('.page-link.next-page-item').click
 
       expect(page).to have_selector('.file-holder', count: 1)
       expect(page).to have_selector('.diff-file .file-title', text: '.gitignore')

spec/frontend/diffs/components/app_spec.js

 import Vuex from 'vuex';
 import { shallowMount, createLocalVue } from '@vue/test-utils';
-import { GlLoadingIcon } from '@gitlab/ui';
+import { GlLoadingIcon, GlPagination } from '@gitlab/ui';
 import MockAdapter from 'axios-mock-adapter';
 import { TEST_HOST } from 'spec/test_constants';
 import Mousetrap from 'mousetrap';
@@ -843,13 +843,16 @@ describe('diffs/components/app', () => {
     });
 
     describe('pagination', () => {
+      const fileByFileNav = () => wrapper.find('[data-testid="file-by-file-navigation"]');
+      const paginator = () => fileByFileNav().find(GlPagination);
+
       it('sets previous button as disabled', () => {
         createComponent({ viewDiffsFileByFile: true }, ({ state }) => {
           state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' });
         });
 
-        expect(wrapper.find('[data-testid="singleFilePrevious"]').props('disabled')).toBe(true);
-        expect(wrapper.find('[data-testid="singleFileNext"]').props('disabled')).toBe(false);
+        expect(paginator().attributes('prevpage')).toBe(undefined);
+        expect(paginator().attributes('nextpage')).toBe('2');
       });
 
       it('sets next button as disabled', () => {
@@ -858,17 +861,26 @@ describe('diffs/components/app', () => {
           state.diffs.currentDiffFileId = '312';
         });
 
-        expect(wrapper.find('[data-testid="singleFilePrevious"]').props('disabled')).toBe(false);
-        expect(wrapper.find('[data-testid="singleFileNext"]').props('disabled')).toBe(true);
+        expect(paginator().attributes('prevpage')).toBe('1');
+        expect(paginator().attributes('nextpage')).toBe(undefined);
+      });
+
+      it("doesn't display when there's fewer than 2 files", () => {
+        createComponent({ viewDiffsFileByFile: true }, ({ state }) => {
+          state.diffs.diffFiles.push({ file_hash: '123' });
+          state.diffs.currentDiffFileId = '123';
+        });
+
+        expect(fileByFileNav().exists()).toBe(false);
       });
 
       it.each`
-        currentDiffFileId | button                  | index
-        ${'123'}          | ${'singleFileNext'}     | ${1}
-        ${'312'}          | ${'singleFilePrevious'} | ${0}
+        currentDiffFileId | targetFile
+        ${'123'}          | ${2}
+        ${'312'}          | ${1}
       `(
-        'it calls navigateToDiffFileIndex with $index when $button is clicked',
-        ({ currentDiffFileId, button, index }) => {
+        'it calls navigateToDiffFileIndex with $index when $link is clicked',
+        async ({ currentDiffFileId, targetFile }) => {
           createComponent({ viewDiffsFileByFile: true }, ({ state }) => {
             state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' });
             state.diffs.currentDiffFileId = currentDiffFileId;
@@ -876,11 +888,11 @@ describe('diffs/components/app', () => {
 
           jest.spyOn(wrapper.vm, 'navigateToDiffFileIndex');
 
-          wrapper.find(`[data-testid="${button}"]`).vm.$emit('click');
+          paginator().vm.$emit('input', targetFile);
 
-          return wrapper.vm.$nextTick().then(() => {
-            expect(wrapper.vm.navigateToDiffFileIndex).toHaveBeenCalledWith(index);
-          });
+          await wrapper.vm.$nextTick();
+
+          expect(wrapper.vm.navigateToDiffFileIndex).toHaveBeenCalledWith(targetFile - 1);
         },
       );
     });