diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 98c660077bbe1f1b734dc6107b52e123e38b3e5e..941365d9d1db516ee8a919f91f40acfcee6ae776 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -224,6 +224,7 @@ export default { methods: { ...mapActions(['startTaskList']), ...mapActions('diffs', [ + 'moveToNeighboringCommit', 'setBaseConfig', 'fetchDiffFiles', 'fetchDiffFilesMeta', @@ -344,9 +345,16 @@ export default { break; } }); + + if (this.commit && this.glFeatures.mrCommitNeighborNav) { + Mousetrap.bind('c', () => this.moveToNeighboringCommit({ direction: 'next' })); + Mousetrap.bind('x', () => this.moveToNeighboringCommit({ direction: 'previous' })); + } }, removeEventListeners() { Mousetrap.unbind(['[', 'k', ']', 'j']); + Mousetrap.unbind('c'); + Mousetrap.unbind('x'); }, jumpToFile(step) { const targetIndex = this.currentDiffIndex + step; diff --git a/app/assets/javascripts/diffs/components/commit_item.vue b/app/assets/javascripts/diffs/components/commit_item.vue index 9d4edd84f25cd864c3c0256927a5449d226b3237..ee93ca020e847a1684915e436b3b5ebf0a24de6e 100644 --- a/app/assets/javascripts/diffs/components/commit_item.vue +++ b/app/assets/javascripts/diffs/components/commit_item.vue @@ -1,10 +1,18 @@ @@ -123,6 +163,41 @@ export default { class="btn btn-default" /> +
+ + + + + {{ __('Prev') }} + + + + {{ __('Next') }} + + + +
diff --git a/app/assets/stylesheets/pages/commits.scss b/app/assets/stylesheets/pages/commits.scss index 230f390aa90167bb677ed9f1afb1153024aeda1f..9a69afc6044e4d9258212114b6df606c9068869a 100644 --- a/app/assets/stylesheets/pages/commits.scss +++ b/app/assets/stylesheets/pages/commits.scss @@ -208,6 +208,18 @@ } } +.commit-nav-buttons { + a.btn, + button { + // See: https://gitlab.com/gitlab-org/gitlab-ui/-/issues/730 + &:last-child > svg { + margin-left: 0.25rem; + margin-right: 0; + } + } +} + + .clipboard-group, .commit-sha-group { display: inline-flex; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index ae7965a2eae33c6a42565161037ff61cef28ae44..6c82a66fb1e31be62a5f0f57422f9f55a172da35 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -27,6 +27,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo push_frontend_feature_flag(:widget_visibility_polling, @project, default_enabled: true) push_frontend_feature_flag(:merge_ref_head_comments, @project) push_frontend_feature_flag(:accessibility_merge_request_widget, @project) + push_frontend_feature_flag(:mr_commit_neighbor_nav, @project, default_enabled: true) end before_action do diff --git a/app/views/help/_shortcuts.html.haml b/app/views/help/_shortcuts.html.haml index aa63127049c9cf6a8a8511e06db58845ef73ff16..bd5424c30c67909f2e1c2cc8ce3839cff5457a19 100644 --- a/app/views/help/_shortcuts.html.haml +++ b/app/views/help/_shortcuts.html.haml @@ -313,6 +313,18 @@ %td.shortcut %kbd p %td= _('Previous unresolved discussion') + %tbody + %tr + %th + %th= _('Merge Request Commits') + %tr + %td.shortcut + %kbd c + %td= _('Next commit') + %tr + %td.shortcut + %kbd x + %td= _('Previous commit') %tbody %tr %th diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml index 8b659034fe64546f22c98c3e4e1c49481de0d61d..b42eef32a7665d40c4dd3ce647547944f14f15d6 100644 --- a/app/views/projects/commits/_commit.html.haml +++ b/app/views/projects/commits/_commit.html.haml @@ -1,6 +1,8 @@ -#----------------------------------------------------------------- WARNING: Please keep changes up-to-date with the following files: - `assets/javascripts/diffs/components/commit_item.vue` + + EXCEPTION WARNING - see above `.vue` file for de-sync drift -#----------------------------------------------------------------- - view_details = local_assigns.fetch(:view_details, false) - merge_request = local_assigns.fetch(:merge_request, nil) diff --git a/changelogs/unreleased/feature-next-prev-commit-buttons.yml b/changelogs/unreleased/feature-next-prev-commit-buttons.yml new file mode 100644 index 0000000000000000000000000000000000000000..3a53f803349701a5e5631334a9f07a781c01c564 --- /dev/null +++ b/changelogs/unreleased/feature-next-prev-commit-buttons.yml @@ -0,0 +1,5 @@ +--- +title: Add Previous and Next buttons for commit-by-commit navigation +merge_request: 28596 +author: +type: added diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 96f90d3beabc10ab48f9c74037ffd0e9371845bb..45f3ea94487b5ed1a6e31f50f05c5abd83565ad3 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -13028,6 +13028,9 @@ msgstr "" msgid "Merge Request Approvals" msgstr "" +msgid "Merge Request Commits" +msgstr "" + msgid "Merge Requests" msgstr "" @@ -13886,6 +13889,9 @@ msgstr "" msgid "Next" msgstr "" +msgid "Next commit" +msgstr "" + msgid "Next file in diff" msgstr "" @@ -15553,6 +15559,9 @@ msgstr "" msgid "Press %{key}-C to copy" msgstr "" +msgid "Prev" +msgstr "" + msgid "Prevent adding new members to project membership within this group" msgstr "" @@ -15589,6 +15598,9 @@ msgstr "" msgid "Previous Artifacts" msgstr "" +msgid "Previous commit" +msgstr "" + msgid "Previous file in diff" msgstr "" @@ -24525,6 +24537,12 @@ msgstr "" msgid "You're about to reduce the visibility of the project %{strong_start}%{project_name}%{strong_end}." msgstr "" +msgid "You're at the first commit" +msgstr "" + +msgid "You're at the last commit" +msgstr "" + msgid "You're not allowed to %{tag_start}edit%{tag_end} files in this project directly. Please fork this project, make your changes there, and submit a merge request." msgstr "" diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 1a95f586344273e871edaad587b3d0e8a60a0b45..57e3a93c6f4472f3f44045547952e6bbac004050 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -28,8 +28,14 @@ describe('diffs/components/app', () => { let wrapper; let mock; - function createComponent(props = {}, extendStore = () => {}) { + function createComponent(props = {}, extendStore = () => {}, provisions = {}) { const localVue = createLocalVue(); + const provide = { + ...provisions, + glFeatures: { + ...(provisions.glFeatures || {}), + }, + }; localVue.use(Vuex); @@ -52,6 +58,7 @@ describe('diffs/components/app', () => { showSuggestPopover: true, ...props, }, + provide, store, methods: { isLatestVersion() { @@ -82,7 +89,10 @@ describe('diffs/components/app', () => { window.mrTabs = oldMrTabs; // reset component - wrapper.destroy(); + if (wrapper) { + wrapper.destroy(); + wrapper = null; + } mock.restore(); }); @@ -455,76 +465,109 @@ describe('diffs/components/app', () => { }); describe('keyboard shortcut navigation', () => { - const mappings = { - '[': -1, - k: -1, - ']': +1, - j: +1, - }; - let spy; + let spies = []; + let jumpSpy; + let moveSpy; + + function setup(componentProps, featureFlags) { + createComponent( + componentProps, + ({ state }) => { + state.diffs.commit = { id: 'SHA123' }; + }, + { glFeatures: { mrCommitNeighborNav: true, ...featureFlags } }, + ); + + moveSpy = jest.spyOn(wrapper.vm, 'moveToNeighboringCommit').mockImplementation(() => {}); + jumpSpy = jest.fn(); + spies = [jumpSpy, moveSpy]; + wrapper.setMethods({ + jumpToFile: jumpSpy, + }); + } describe('visible app', () => { - beforeEach(() => { - spy = jest.fn(); + it.each` + key | name | spy | args | featureFlags + ${'['} | ${'jumpToFile'} | ${0} | ${[-1]} | ${{}} + ${'k'} | ${'jumpToFile'} | ${0} | ${[-1]} | ${{}} + ${']'} | ${'jumpToFile'} | ${0} | ${[+1]} | ${{}} + ${'j'} | ${'jumpToFile'} | ${0} | ${[+1]} | ${{}} + ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'previous' }]} | ${{ mrCommitNeighborNav: true }} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${[{ direction: 'next' }]} | ${{ mrCommitNeighborNav: true }} + `( + 'calls `$name()` with correct parameters whenever the "$key" key is pressed', + ({ key, spy, args, featureFlags }) => { + setup({ shouldShow: true }, featureFlags); - createComponent({ - shouldShow: true, - }); - wrapper.setMethods({ - jumpToFile: spy, - }); - }); + return wrapper.vm.$nextTick().then(() => { + expect(spies[spy]).not.toHaveBeenCalled(); + + Mousetrap.trigger(key); + + expect(spies[spy]).toHaveBeenCalledWith(...args); + }); + }, + ); + + it.each` + key | name | spy | featureFlags + ${'x'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} | ${{ mrCommitNeighborNav: false }} + `( + 'does not call `$name()` even when the correct key is pressed if the feature flag is disabled', + ({ key, spy, featureFlags }) => { + setup({ shouldShow: true }, featureFlags); - it.each(Object.keys(mappings))( - 'calls `jumpToFile()` with correct parameter whenever pre-defined %s is pressed', - key => { return wrapper.vm.$nextTick().then(() => { - expect(spy).not.toHaveBeenCalled(); + expect(spies[spy]).not.toHaveBeenCalled(); Mousetrap.trigger(key); - expect(spy).toHaveBeenCalledWith(mappings[key]); + expect(spies[spy]).not.toHaveBeenCalled(); }); }, ); - it('does not call `jumpToFile()` when unknown key is pressed', done => { - wrapper.vm - .$nextTick() - .then(() => { - Mousetrap.trigger('d'); + it.each` + key | name | spy | allowed + ${'d'} | ${'jumpToFile'} | ${0} | ${['[', ']', 'j', 'k']} + ${'r'} | ${'moveToNeighboringCommit'} | ${1} | ${['x', 'c']} + `( + `does not call \`$name()\` when a key that is not one of \`$allowed\` is pressed`, + ({ key, spy }) => { + setup({ shouldShow: true }, { mrCommitNeighborNav: true }); - expect(spy).not.toHaveBeenCalled(); - }) - .then(done) - .catch(done.fail); - }); + return wrapper.vm.$nextTick().then(() => { + Mousetrap.trigger(key); + + expect(spies[spy]).not.toHaveBeenCalled(); + }); + }, + ); }); - describe('hideen app', () => { + describe('hidden app', () => { beforeEach(() => { - spy = jest.fn(); + setup({ shouldShow: false }, { mrCommitNeighborNav: true }); - createComponent({ - shouldShow: false, - }); - wrapper.setMethods({ - jumpToFile: spy, + return wrapper.vm.$nextTick().then(() => { + Mousetrap.reset(); }); }); - it('stops calling `jumpToFile()` when application is hidden', done => { - wrapper.vm - .$nextTick() - .then(() => { - Object.keys(mappings).forEach(key => { - Mousetrap.trigger(key); + it.each` + key | name | spy + ${'['} | ${'jumpToFile'} | ${0} + ${'k'} | ${'jumpToFile'} | ${0} + ${']'} | ${'jumpToFile'} | ${0} + ${'j'} | ${'jumpToFile'} | ${0} + ${'x'} | ${'moveToNeighboringCommit'} | ${1} + ${'c'} | ${'moveToNeighboringCommit'} | ${1} + `('stops calling `$name()` when the app is hidden', ({ key, spy }) => { + Mousetrap.trigger(key); - expect(spy).not.toHaveBeenCalled(); - }); - }) - .then(done) - .catch(done.fail); + expect(spies[spy]).not.toHaveBeenCalled(); }); }); }); diff --git a/spec/frontend/diffs/components/commit_item_spec.js b/spec/frontend/diffs/components/commit_item_spec.js index 6bb3a0dcf214a059042de5f18c53b0c101a196cb..0df951d43a70d39950071cced4fd254b698226d4 100644 --- a/spec/frontend/diffs/components/commit_item_spec.js +++ b/spec/frontend/diffs/components/commit_item_spec.js @@ -13,6 +13,8 @@ const TEST_AUTHOR_EMAIL = 'test+test@gitlab.com'; const TEST_AUTHOR_GRAVATAR = `${TEST_HOST}/avatar/test?s=40`; const TEST_SIGNATURE_HTML = 'Legit commit'; 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; @@ -30,12 +32,24 @@ describe('diffs/components/commit_item', () => { const getCommitActionsElement = () => wrapper.find('.commit-actions'); const getCommitPipelineStatus = () => wrapper.find(CommitPipelineStatus); - const mountComponent = propsData => { + 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, featureFlags = {}) => { wrapper = mount(Component, { propsData: { commit, ...propsData, }, + provide: { + glFeatures: { + mrCommitNeighborNav: true, + ...featureFlags, + }, + }, stubs: { CommitPipelineStatus: true, }, @@ -173,4 +187,132 @@ 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); + }); + + it('does not render the commit navigation buttons if the `mrCommitNeighborNav` feature flag is disabled', () => { + mountComponent({ commit: mrCommit }, { mrCommitNeighborNav: false }); + + expect(getCommitNavButtonsElement().exists()).toEqual(false); + }); + + 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); + }); + }); + }); });