From 6aea0191c7478b64f53a08783767b1d6e3f39bf0 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 18 Aug 2020 21:41:21 -0600
Subject: [PATCH 1/7] Switch file-by-file nav to use the Pagination component

---
 .../javascripts/diffs/components/app.vue      | 38 +++++++++----------
 .../stylesheets/pages/merge_requests.scss     |  5 +++
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 5062006424eff7..0c49fac09d9564 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -1,6 +1,6 @@
 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
-import { GlLoadingIcon, GlButtonGroup, GlButton, GlAlert } from '@gitlab/ui';
+import { GlLoadingIcon, GlButton, GlAlert, GlPagination } from '@gitlab/ui';
 import Mousetrap from 'mousetrap';
 import { __ } from '~/locale';
 import { getParameterByName, parseBoolean } from '~/lib/utils/common_utils';
@@ -37,7 +37,7 @@ export default {
     TreeList,
     GlLoadingIcon,
     PanelResizer,
-    GlButtonGroup,
+    GlPagination,
     GlButton,
     GlAlert,
   },
@@ -169,6 +169,16 @@ export default {
     isDiffHead() {
       return parseBoolean(getParameterByName('diff_head'));
     },
+    previousFile() {
+      const currentFile = this.currentDiffIndex + 1;
+
+      return currentFile >= 2 ? currentFile - 1 : null;
+    },
+    nextFile() {
+      const currentFile = this.currentDiffIndex + 1;
+
+      return currentFile < this.diffFiles.length ? currentFile + 1 : null;
+    },
   },
   watch: {
     commit(newCommit, oldCommit) {
@@ -509,23 +519,13 @@ 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="viewDiffsFileByFile" class="file-by-file-navigation">
+              <gl-pagination
+                :value="currentDiffIndex + 1"
+                :prev-page="previousFile"
+                :next-page="nextFile"
+                @input="val => navigateToDiffFileIndex(val - 1)"
+              />
             </div>
           </template>
           <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" />
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index a6d1fc11c3f7ad..43a468e28ae802 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -1035,3 +1035,8 @@ $mr-widget-min-height: 69px;
 .diff-file-row.is-active {
   background-color: $gray-50;
 }
+
+.file-by-file-navigation {
+  display: grid;
+  place-items: center;
+}
-- 
GitLab


From 0ffdf51ddc5e162901758016f5b52ca589d9b441 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 20 Aug 2020 05:41:31 -0600
Subject: [PATCH 2/7] Add current file and total files to the file-by-file nav

---
 .../javascripts/diffs/components/app.vue      | 43 +++++++++++++------
 .../stylesheets/pages/merge_requests.scss     |  5 ---
 2 files changed, 31 insertions(+), 17 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 0c49fac09d9564..3680bebfdd0d74 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -1,6 +1,6 @@
 <script>
 import { mapState, mapGetters, mapActions } from 'vuex';
-import { GlLoadingIcon, GlButton, GlAlert, GlPagination } 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';
@@ -40,6 +40,7 @@ export default {
     GlPagination,
     GlButton,
     GlAlert,
+    GlSprintf,
   },
   mixins: [glFeatureFlagsMixin()],
   props: {
@@ -169,15 +170,21 @@ export default {
     isDiffHead() {
       return parseBoolean(getParameterByName('diff_head'));
     },
-    previousFile() {
-      const currentFile = this.currentDiffIndex + 1;
+    showFileByFileNavigation() {
+      return this.diffFiles.length > 1 && this.viewDiffsFileByFile;
+    },
+    currentFileNumber() {
+      return this.currentDiffIndex + 1;
+    },
+    previousFileNumber() {
+      const { currentDiffIndex } = this;
 
-      return currentFile >= 2 ? currentFile - 1 : null;
+      return currentDiffIndex >= 1 ? currentDiffIndex : null;
     },
-    nextFile() {
-      const currentFile = this.currentDiffIndex + 1;
+    nextFileNumber() {
+      const { currentFileNumber, diffFiles } = this;
 
-      return currentFile < this.diffFiles.length ? currentFile + 1 : null;
+      return currentFileNumber < diffFiles.length ? currentFileNumber + 1 : null;
     },
   },
   watch: {
@@ -284,6 +291,9 @@ export default {
       'toggleShowTreeList',
       'navigateToDiffFileIndex',
     ]),
+    navigateToDiffFileNumber(number) {
+      this.navigateToDiffFileIndex(number - 1);
+    },
     refetchDiffData() {
       this.fetchData(false);
     },
@@ -519,13 +529,22 @@ export default {
               :can-current-user-fork="canCurrentUserFork"
               :view-diffs-file-by-file="viewDiffsFileByFile"
             />
-            <div v-if="viewDiffsFileByFile" class="file-by-file-navigation">
+            <div
+              v-if="showFileByFileNavigation"
+              data-testid="file-by-file-navigation"
+              class="gl-display-grid gl-text-center"
+            >
               <gl-pagination
-                :value="currentDiffIndex + 1"
-                :prev-page="previousFile"
-                :next-page="nextFile"
-                @input="val => navigateToDiffFileIndex(val - 1)"
+                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" />
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 43a468e28ae802..a6d1fc11c3f7ad 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -1035,8 +1035,3 @@ $mr-widget-min-height: 69px;
 .diff-file-row.is-active {
   background-color: $gray-50;
 }
-
-.file-by-file-navigation {
-  display: grid;
-  place-items: center;
-}
-- 
GitLab


From ff156d488104f263f5fa05c05362de30ca52b851 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 20 Aug 2020 05:50:31 -0600
Subject: [PATCH 3/7] Test using the pagination component instead of custom
 buttons

---
 spec/frontend/diffs/components/app_spec.js | 31 ++++++++++++----------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js
index ac046ddc20319f..ee0897ee256390 100644
--- a/spec/frontend/diffs/components/app_spec.js
+++ b/spec/frontend/diffs/components/app_spec.js
@@ -1,6 +1,6 @@
 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,17 @@ 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.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 +879,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);
         },
       );
     });
-- 
GitLab


From 13e2f375ef0cfb5d0ca230f9668721420dc2e49a Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 19 Aug 2020 03:14:59 -0600
Subject: [PATCH 4/7] Add text for file-by-file navigation label

---
 locale/gitlab.pot | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 050b08dc32f804..be3835179298df 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -10660,6 +10660,9 @@ msgstr ""
 msgid "File"
 msgstr ""
 
+msgid "File %{current} of %{total}"
+msgstr ""
+
 msgid "File Hooks"
 msgstr ""
 
-- 
GitLab


From e238de2a3ebf4974cf7684aff3441cc81f1f1aec Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Thu, 20 Aug 2020 05:41:10 -0600
Subject: [PATCH 5/7] Update existing tests for file-by-file nav

---
 .../merge_request/user_views_auto_expanding_diff_spec.rb       | 3 ++-
 .../merge_request/user_views_diffs_file_by_file_spec.rb        | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/spec/features/merge_request/user_views_auto_expanding_diff_spec.rb b/spec/features/merge_request/user_views_auto_expanding_diff_spec.rb
index 20a5910e66deae..585a389157e425 100644
--- a/spec/features/merge_request/user_views_auto_expanding_diff_spec.rb
+++ b/spec/features/merge_request/user_views_auto_expanding_diff_spec.rb
@@ -27,9 +27,10 @@
     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
diff --git a/spec/features/merge_request/user_views_diffs_file_by_file_spec.rb b/spec/features/merge_request/user_views_diffs_file_by_file_spec.rb
index abb313cb529a7d..bb4bf0864c963e 100644
--- a/spec/features/merge_request/user_views_diffs_file_by_file_spec.rb
+++ b/spec/features/merge_request/user_views_diffs_file_by_file_spec.rb
@@ -25,7 +25,7 @@
       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')
-- 
GitLab


From 95c7331284ca831f90492c41d40484d46cdd19c1 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 19 Aug 2020 07:54:56 -0600
Subject: [PATCH 6/7] Add changelog for the updated file-by-file nav

---
 changelogs/unreleased/maintenance-improve-file-by-file.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/maintenance-improve-file-by-file.yml

diff --git a/changelogs/unreleased/maintenance-improve-file-by-file.yml b/changelogs/unreleased/maintenance-improve-file-by-file.yml
new file mode 100644
index 00000000000000..56b1668f0b1c55
--- /dev/null
+++ b/changelogs/unreleased/maintenance-improve-file-by-file.yml
@@ -0,0 +1,5 @@
+---
+title: Tweak file-by-file display and add file current/total display
+merge_request: 39719
+author:
+type: changed
-- 
GitLab


From 6d61940d568ea9ed2bcf5b8c64d1bee580bde121 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 19 Aug 2020 08:44:49 -0600
Subject: [PATCH 7/7] Test that the pagination is removed when there's only one
 file

---
 spec/frontend/diffs/components/app_spec.js | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js
index ee0897ee256390..1f274456ded934 100644
--- a/spec/frontend/diffs/components/app_spec.js
+++ b/spec/frontend/diffs/components/app_spec.js
@@ -865,6 +865,15 @@ describe('diffs/components/app', () => {
         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 | targetFile
         ${'123'}          | ${2}
-- 
GitLab