Merge branch 'tc-move-commit-buttons' into 'master'

Move commit neighbor buttons to sticky MR controls

See merge request gitlab-org/gitlab!57743

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);
+      });
+    });
+  });
 });