Move commit neighbor buttons to sticky MR controls

When reviewing an MR commit by commit, there are `Prev` and `Next`
buttons next to the commit message to jump to the neighbor
commits. These are helpful, but they slide outside of view when
scrolling to the bottom.

These buttons were initially introduced in [1].

This changes moves these buttons to the sticky Merge Request controls
bar. This bar remains visible at all times, making the neighbor commit
buttons accessible even when scrolled to the bottom.

1. ee921003 (Add buttons to the Commit Item to navigate among MR
   commits, 2020-05-12)

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

 <script>
 /* eslint-disable vue/no-v-html */
-import { GlButtonGroup, GlButton, GlTooltipDirective, GlIcon } from '@gitlab/ui';
-import { mapActions } from 'vuex';
+import { GlButtonGroup, GlButton, GlTooltipDirective } from '@gitlab/ui';
 
 import CommitPipelineStatus from '~/projects/tree/components/commit_pipeline_status_component.vue';
 import ModalCopyButton from '~/vue_shared/components/modal_copy_button.vue';
@@ -9,7 +8,6 @@ import TimeAgoTooltip from '~/vue_shared/components/time_ago_tooltip.vue';
 import UserAvatarLink from '~/vue_shared/components/user_avatar/user_avatar_link.vue';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 
-import { setUrlParams } from '../../lib/utils/url_utility';
 import initUserPopovers from '../../user_popovers';
 
 /**
@@ -24,14 +22,6 @@ import initUserPopovers from '../../user_popovers';
  * coexist, but there is an issue to remove the duplication.
  * https://gitlab.com/gitlab-org/gitlab-foss/issues/51613
  *
- * EXCEPTION WARNING
- * 1. The commit navigation buttons (next neighbor, previous neighbor)
- *    are not duplicated because:
- *     - We don't have the same data available on the Rails side (yet,
- *        without backend work)
- *     - This Vue component should always be what's used when in the
- *        context of an MR diff, so the HAML should never have any idea
- *        about navigating among commits.
  */
 
 export default {
@@ -42,7 +32,6 @@ export default {
     CommitPipelineStatus,
     GlButtonGroup,
     GlButton,
-    GlIcon,
   },
   directives: {
     GlTooltip: GlTooltipDirective,
@@ -94,28 +83,12 @@ export default {
       // Strip the newline at the beginning
       return this.commit.description_html.replace(/^&#x000A;/, '');
     },
-    nextCommitUrl() {
-      return this.commit.next_commit_id
-        ? setUrlParams({ commit_id: this.commit.next_commit_id })
-        : '';
-    },
-    previousCommitUrl() {
-      return this.commit.prev_commit_id
-        ? setUrlParams({ commit_id: this.commit.prev_commit_id })
-        : '';
-    },
-    hasNeighborCommits() {
-      return this.commit.next_commit_id || this.commit.prev_commit_id;
-    },
   },
   created() {
     this.$nextTick(() => {
       initUserPopovers(this.$el.querySelectorAll('.js-user-link'));
     });
   },
-  methods: {
-    ...mapActions('diffs', ['moveToNeighboringCommit']),
-  },
 };
 </script>
 
@@ -146,38 +119,6 @@ export default {
             class="input-group-text"
           />
         </gl-button-group>
-        <div v-if="hasNeighborCommits" class="commit-nav-buttons ml-3">
-          <gl-button-group>
-            <gl-button
-              :href="previousCommitUrl"
-              :disabled="!commit.prev_commit_id"
-              @click.prevent="moveToNeighboringCommit({ direction: 'previous' })"
-            >
-              <span
-                v-if="!commit.prev_commit_id"
-                v-gl-tooltip
-                class="h-100 w-100 position-absolute"
-                :title="__('You\'re at the first commit')"
-              ></span>
-              <gl-icon name="chevron-left" />
-              {{ __('Prev') }}
-            </gl-button>
-            <gl-button
-              :href="nextCommitUrl"
-              :disabled="!commit.next_commit_id"
-              @click.prevent="moveToNeighboringCommit({ direction: 'next' })"
-            >
-              <span
-                v-if="!commit.next_commit_id"
-                v-gl-tooltip
-                class="h-100 w-100 position-absolute"
-                :title="__('You\'re at the last commit')"
-              ></span>
-              {{ __('Next') }}
-              <gl-icon name="chevron-right" />
-            </gl-button>
-          </gl-button-group>
-        </div>
       </div>
       <div>
         <div class="d-flex float-left align-items-center align-self-start">

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

 <script>
-import { GlTooltipDirective, GlLink, GlButton, GlSprintf } from '@gitlab/ui';
+import { GlTooltipDirective, GlIcon, GlLink, GlButtonGroup, GlButton, GlSprintf } from '@gitlab/ui';
 import { mapActions, mapGetters, mapState } from 'vuex';
 import { __ } from '~/locale';
+import { setUrlParams } from '../../lib/utils/url_utility';
 import { CENTERED_LIMITED_CONTAINER_CLASSES, EVT_EXPAND_ALL_FILES } from '../constants';
 import eventHub from '../event_hub';
 import CompareDropdownLayout from './compare_dropdown_layout.vue';
@@ -11,7 +12,9 @@ import SettingsDropdown from './settings_dropdown.vue';
 export default {
   components: {
     CompareDropdownLayout,
+    GlIcon,
     GlLink,
+    GlButtonGroup,
     GlButton,
     GlSprintf,
     SettingsDropdown,
@@ -56,6 +59,19 @@ export default {
     hasSourceVersions() {
       return this.diffCompareDropdownSourceVersions.length > 0;
     },
+    nextCommitUrl() {
+      return this.commit.next_commit_id
+        ? setUrlParams({ commit_id: this.commit.next_commit_id })
+        : '';
+    },
+    previousCommitUrl() {
+      return this.commit.prev_commit_id
+        ? setUrlParams({ commit_id: this.commit.prev_commit_id })
+        : '';
+    },
+    hasNeighborCommits() {
+      return this.commit && (this.commit.next_commit_id || this.commit.prev_commit_id);
+    },
   },
   created() {
     this.CENTERED_LIMITED_CONTAINER_CLASSES = CENTERED_LIMITED_CONTAINER_CLASSES;
@@ -65,6 +81,7 @@ export default {
     expandAllFiles() {
       eventHub.$emit(EVT_EXPAND_ALL_FILES);
     },
+    ...mapActions('diffs', ['moveToNeighboringCommit']),
   },
 };
 </script>
@@ -92,6 +109,38 @@ export default {
         {{ __('Viewing commit') }}
         <gl-link :href="commit.commit_url" class="monospace">{{ commit.short_id }}</gl-link>
       </div>
+      <div v-if="hasNeighborCommits" class="commit-nav-buttons ml-3">
+        <gl-button-group>
+          <gl-button
+            :href="previousCommitUrl"
+            :disabled="!commit.prev_commit_id"
+            @click.prevent="moveToNeighboringCommit({ direction: 'previous' })"
+          >
+            <span
+              v-if="!commit.prev_commit_id"
+              v-gl-tooltip
+              class="h-100 w-100 position-absolute position-top-0 position-left-0"
+              :title="__('You\'re at the first commit')"
+            ></span>
+            <gl-icon name="chevron-left" />
+            {{ __('Prev') }}
+          </gl-button>
+          <gl-button
+            :href="nextCommitUrl"
+            :disabled="!commit.next_commit_id"
+            @click.prevent="moveToNeighboringCommit({ direction: 'next' })"
+          >
+            <span
+              v-if="!commit.next_commit_id"
+              v-gl-tooltip
+              class="h-100 w-100 position-absolute position-top-0 position-left-0"
+              :title="__('You\'re at the last commit')"
+            ></span>
+            {{ __('Next') }}
+            <gl-icon name="chevron-right" />
+          </gl-button>
+        </gl-button-group>
+      </div>
       <gl-sprintf
         v-else-if="hasSourceVersions"
         class="d-flex align-items-center compare-versions-container"

changelogs/unreleased/tc-move-commit-buttons.yml

+---
+title: Move commit neighbor buttons to sticky MR controls
+merge_request: 57743
+author:
+type: changed

doc/user/project/merge_requests/reviewing_and_managing_merge_requests.md

@@ -145,10 +145,10 @@ To seamlessly navigate among commits in a merge request:
 1. Select a commit to open it in the single-commit view.
 1. Navigate through the commits by either:
 
-   - Selecting **Prev** and **Next** buttons on the top-right of the page.
+   - Selecting **Prev** and **Next** buttons right beneath the tab buttons.
    - Using the <kbd>X</kbd> and <kbd>C</kbd> keyboard shortcuts.
 
-![Merge requests commit navigation](img/commit_nav_v13_4.png)
+![Merge requests commit navigation](img/commit_nav_v13_11.png)
 
 ### Incrementally expand merge request diffs
 

spec/frontend/diffs/components/commit_item_spec.js

@@ -13,8 +13,6 @@ const TEST_AUTHOR_EMAIL = 'test+test@gitlab.com';
 const TEST_AUTHOR_GRAVATAR = `${TEST_HOST}/avatar/test?s=40`;
 const TEST_SIGNATURE_HTML = '<a>Legit commit</a>';
 const TEST_PIPELINE_STATUS_PATH = `${TEST_HOST}/pipeline/status`;
-const NEXT_COMMIT_URL = `${TEST_HOST}/?commit_id=next`;
-const PREV_COMMIT_URL = `${TEST_HOST}/?commit_id=prev`;
 
 describe('diffs/components/commit_item', () => {
   let wrapper;
@@ -31,12 +29,6 @@ describe('diffs/components/commit_item', () => {
   const getCommitActionsElement = () => wrapper.find('.commit-actions');
   const getCommitPipelineStatus = () => wrapper.find(CommitPipelineStatus);
 
-  const getCommitNavButtonsElement = () => wrapper.find('.commit-nav-buttons');
-  const getNextCommitNavElement = () =>
-    getCommitNavButtonsElement().find('.btn-group > *:last-child');
-  const getPrevCommitNavElement = () =>
-    getCommitNavButtonsElement().find('.btn-group > *:first-child');
-
   const mountComponent = (propsData) => {
     wrapper = mount(Component, {
       propsData: {
@@ -180,126 +172,4 @@ describe('diffs/components/commit_item', () => {
       expect(getCommitPipelineStatus().exists()).toBe(true);
     });
   });
-
-  describe('without neighbor commits', () => {
-    beforeEach(() => {
-      mountComponent({ commit: { ...commit, prev_commit_id: null, next_commit_id: null } });
-    });
-
-    it('does not render any navigation buttons', () => {
-      expect(getCommitNavButtonsElement().exists()).toEqual(false);
-    });
-  });
-
-  describe('with neighbor commits', () => {
-    let mrCommit;
-
-    beforeEach(() => {
-      mrCommit = {
-        ...commit,
-        next_commit_id: 'next',
-        prev_commit_id: 'prev',
-      };
-
-      mountComponent({ commit: mrCommit });
-    });
-
-    it('renders the commit navigation buttons', () => {
-      expect(getCommitNavButtonsElement().exists()).toEqual(true);
-
-      mountComponent({
-        commit: { ...mrCommit, next_commit_id: null },
-      });
-      expect(getCommitNavButtonsElement().exists()).toEqual(true);
-
-      mountComponent({
-        commit: { ...mrCommit, prev_commit_id: null },
-      });
-      expect(getCommitNavButtonsElement().exists()).toEqual(true);
-    });
-
-    describe('prev commit', () => {
-      const { location } = window;
-
-      beforeAll(() => {
-        delete window.location;
-        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
-      });
-
-      beforeEach(() => {
-        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
-      });
-
-      afterAll(() => {
-        window.location = location;
-      });
-
-      it('uses the correct href', () => {
-        const link = getPrevCommitNavElement();
-
-        expect(link.element.getAttribute('href')).toEqual(PREV_COMMIT_URL);
-      });
-
-      it('triggers the correct Vuex action on click', () => {
-        const link = getPrevCommitNavElement();
-
-        link.trigger('click');
-        return wrapper.vm.$nextTick().then(() => {
-          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({
-            direction: 'previous',
-          });
-        });
-      });
-
-      it('renders a disabled button when there is no prev commit', () => {
-        mountComponent({ commit: { ...mrCommit, prev_commit_id: null } });
-
-        const button = getPrevCommitNavElement();
-
-        expect(button.element.tagName).toEqual('BUTTON');
-        expect(button.element.hasAttribute('disabled')).toEqual(true);
-      });
-    });
-
-    describe('next commit', () => {
-      const { location } = window;
-
-      beforeAll(() => {
-        delete window.location;
-        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
-      });
-
-      beforeEach(() => {
-        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
-      });
-
-      afterAll(() => {
-        window.location = location;
-      });
-
-      it('uses the correct href', () => {
-        const link = getNextCommitNavElement();
-
-        expect(link.element.getAttribute('href')).toEqual(NEXT_COMMIT_URL);
-      });
-
-      it('triggers the correct Vuex action on click', () => {
-        const link = getNextCommitNavElement();
-
-        link.trigger('click');
-        return wrapper.vm.$nextTick().then(() => {
-          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({ direction: 'next' });
-        });
-      });
-
-      it('renders a disabled button when there is no next commit', () => {
-        mountComponent({ commit: { ...mrCommit, next_commit_id: null } });
-
-        const button = getNextCommitNavElement();
-
-        expect(button.element.tagName).toEqual('BUTTON');
-        expect(button.element.hasAttribute('disabled')).toEqual(true);
-      });
-    });
-  });
 });

spec/frontend/diffs/components/compare_versions_spec.js

 import { mount, createLocalVue } from '@vue/test-utils';
 import Vuex from 'vuex';
+import { TEST_HOST } from 'helpers/test_constants';
 import { trimText } from 'helpers/text_helper';
 import CompareVersionsComponent from '~/diffs/components/compare_versions.vue';
 import { createStore } from '~/mr_notes/stores';
@@ -9,12 +10,17 @@ import diffsMockData from '../mock_data/merge_request_diffs';
 const localVue = createLocalVue();
 localVue.use(Vuex);
 
+const NEXT_COMMIT_URL = `${TEST_HOST}/?commit_id=next`;
+const PREV_COMMIT_URL = `${TEST_HOST}/?commit_id=prev`;
+
 describe('CompareVersions', () => {
   let wrapper;
   let store;
   const targetBranchName = 'tmp-wine-dev';
+  const { commit } = getDiffWithCommit();
 
-  const createWrapper = (props) => {
+  const createWrapper = (props = {}, commitArgs = {}) => {
+    store.state.diffs.commit = { ...store.state.diffs.commit, ...commitArgs };
     wrapper = mount(CompareVersionsComponent, {
       localVue,
       store,
@@ -28,6 +34,11 @@ describe('CompareVersions', () => {
   const findLimitedContainer = () => wrapper.find('.container-limited.limit-container-width');
   const findCompareSourceDropdown = () => wrapper.find('.mr-version-dropdown');
   const findCompareTargetDropdown = () => wrapper.find('.mr-version-compare-dropdown');
+  const getCommitNavButtonsElement = () => wrapper.find('.commit-nav-buttons');
+  const getNextCommitNavElement = () =>
+    getCommitNavButtonsElement().find('.btn-group > *:last-child');
+  const getPrevCommitNavElement = () =>
+    getCommitNavButtonsElement().find('.btn-group > *:first-child');
 
   beforeEach(() => {
     store = createStore();
@@ -161,4 +172,124 @@ describe('CompareVersions', () => {
       expect(findCompareTargetDropdown().exists()).toBe(false);
     });
   });
+
+  describe('without neighbor commits', () => {
+    beforeEach(() => {
+      createWrapper({ commit: { ...commit, prev_commit_id: null, next_commit_id: null } });
+    });
+
+    it('does not render any navigation buttons', () => {
+      expect(getCommitNavButtonsElement().exists()).toEqual(false);
+    });
+  });
+
+  describe('with neighbor commits', () => {
+    let mrCommit;
+
+    beforeEach(() => {
+      mrCommit = {
+        ...commit,
+        next_commit_id: 'next',
+        prev_commit_id: 'prev',
+      };
+
+      createWrapper({}, mrCommit);
+    });
+
+    it('renders the commit navigation buttons', () => {
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+
+      createWrapper({
+        commit: { ...mrCommit, next_commit_id: null },
+      });
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+
+      createWrapper({
+        commit: { ...mrCommit, prev_commit_id: null },
+      });
+      expect(getCommitNavButtonsElement().exists()).toEqual(true);
+    });
+
+    describe('prev commit', () => {
+      const { location } = window;
+
+      beforeAll(() => {
+        delete window.location;
+        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
+      });
+
+      beforeEach(() => {
+        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
+      });
+
+      afterAll(() => {
+        window.location = location;
+      });
+
+      it('uses the correct href', () => {
+        const link = getPrevCommitNavElement();
+
+        expect(link.element.getAttribute('href')).toEqual(PREV_COMMIT_URL);
+      });
+
+      it('triggers the correct Vuex action on click', () => {
+        const link = getPrevCommitNavElement();
+
+        link.trigger('click');
+        return wrapper.vm.$nextTick().then(() => {
+          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({
+            direction: 'previous',
+          });
+        });
+      });
+
+      it('renders a disabled button when there is no prev commit', () => {
+        createWrapper({}, { ...mrCommit, prev_commit_id: null });
+
+        const button = getPrevCommitNavElement();
+
+        expect(button.element.hasAttribute('disabled')).toEqual(true);
+      });
+    });
+
+    describe('next commit', () => {
+      const { location } = window;
+
+      beforeAll(() => {
+        delete window.location;
+        window.location = { href: `${TEST_HOST}?commit_id=${mrCommit.id}` };
+      });
+
+      beforeEach(() => {
+        jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {});
+      });
+
+      afterAll(() => {
+        window.location = location;
+      });
+
+      it('uses the correct href', () => {
+        const link = getNextCommitNavElement();
+
+        expect(link.element.getAttribute('href')).toEqual(NEXT_COMMIT_URL);
+      });
+
+      it('triggers the correct Vuex action on click', () => {
+        const link = getNextCommitNavElement();
+
+        link.trigger('click');
+        return wrapper.vm.$nextTick().then(() => {
+          expect(wrapper.vm.moveToNeighboringCommit).toHaveBeenCalledWith({ direction: 'next' });
+        });
+      });
+
+      it('renders a disabled button when there is no next commit', () => {
+        createWrapper({}, { ...mrCommit, next_commit_id: null });
+
+        const button = getNextCommitNavElement();
+
+        expect(button.element.hasAttribute('disabled')).toEqual(true);
+      });
+    });
+  });
 });