From c9a42f1657723cd58c245cb3a23492ea86e38910 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 28 Oct 2020 15:36:03 -0600 Subject: [PATCH 1/2] Use an event hub for MR Diffs to coordinate all expandAll requests Instead of changing a value in state that is then observed in components, the components just subscribe to the event they know they need to handle and perform the "normal" toggle behavior. --- .../components/collapsed_files_warning.vue | 6 ++--- .../diffs/components/compare_versions.vue | 5 +++- .../diffs/components/diff_file.vue | 14 ++++++++-- app/assets/javascripts/diffs/event_hub.js | 3 +++ .../collapsed_files_warning_spec.js | 7 ++--- .../diffs/components/diff_file_spec.js | 26 +++++++++++++++++++ 6 files changed, 51 insertions(+), 10 deletions(-) create mode 100644 app/assets/javascripts/diffs/event_hub.js diff --git a/app/assets/javascripts/diffs/components/collapsed_files_warning.vue b/app/assets/javascripts/diffs/components/collapsed_files_warning.vue index 270bbfb99b72b5..1d4168449b2bf2 100644 --- a/app/assets/javascripts/diffs/components/collapsed_files_warning.vue +++ b/app/assets/javascripts/diffs/components/collapsed_files_warning.vue @@ -1,9 +1,8 @@ diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 564d64c38c426c..6d74952b7a16a3 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -6,13 +6,14 @@ import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { sprintf } from '~/locale'; import { deprecatedCreateFlash as createFlash } from '~/flash'; import { hasDiff } from '~/helpers/diffs_helper'; -import eventHub from '../../notes/event_hub'; +import notesEventHub from '../../notes/event_hub'; import DiffFileHeader from './diff_file_header.vue'; import DiffContent from './diff_content.vue'; import { diffViewerErrors } from '~/ide/constants'; import { collapsedType, isCollapsed } from '../diff_file'; import { DIFF_FILE_AUTOMATIC_COLLAPSE, DIFF_FILE_MANUAL_COLLAPSE } from '../constants'; import { DIFF_FILE, GENERIC_ERROR } from '../i18n'; +import eventHub from '../event_hub'; export default { components: { @@ -151,7 +152,11 @@ export default { }, }, created() { - eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.requestDiff); + notesEventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.requestDiff); + eventHub.$on('mr:diffs:expandAllFiles', this.expandAllListener); + }, + beforeDestroy() { + eventHub.$off('mr:diffs:expandAllFiles', this.expandAllListener); }, methods: { ...mapActions('diffs', [ @@ -160,6 +165,11 @@ export default { 'setRenderIt', 'setFileCollapsedByUser', ]), + expandAllListener() { + if (this.isCollapsed) { + this.handleToggle(); + } + }, handleToggle() { const currentCollapsedFlag = this.isCollapsed; diff --git a/app/assets/javascripts/diffs/event_hub.js b/app/assets/javascripts/diffs/event_hub.js new file mode 100644 index 00000000000000..3e0c313f5e85f8 --- /dev/null +++ b/app/assets/javascripts/diffs/event_hub.js @@ -0,0 +1,3 @@ +import eventHubFactory from '~/helpers/event_hub_factory'; + +export default eventHubFactory(); diff --git a/spec/frontend/diffs/components/collapsed_files_warning_spec.js b/spec/frontend/diffs/components/collapsed_files_warning_spec.js index 7bbffb7a1cd4ad..fbaaf8cf5a69b1 100644 --- a/spec/frontend/diffs/components/collapsed_files_warning_spec.js +++ b/spec/frontend/diffs/components/collapsed_files_warning_spec.js @@ -3,6 +3,7 @@ import { shallowMount, mount, createLocalVue } from '@vue/test-utils'; import createStore from '~/diffs/store/modules'; import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vue'; import { CENTERED_LIMITED_CONTAINER_CLASSES } from '~/diffs/constants'; +import eventHub from '~/diffs/event_hub'; const propsData = { limited: true, @@ -76,13 +77,13 @@ describe('CollapsedFilesWarning', () => { expect(wrapper.find('[data-testid="root"]').exists()).toBe(false); }); - it('triggers the expandAllFiles action when the alert action button is clicked', () => { + it('emits the `mr:diffs:expandAllFiles` event when the alert action button is clicked', () => { createComponent({}, { full: true }); - jest.spyOn(wrapper.vm.$store, 'dispatch').mockReturnValue(undefined); + jest.spyOn(eventHub, '$emit'); getAlertActionButton().vm.$emit('click'); - expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/expandAllFiles', undefined); + expect(eventHub.$emit).toHaveBeenCalledWith('mr:diffs:expandAllFiles'); }); }); diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index 59aa6d811eee4d..4a41b5b4f9864f 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -9,6 +9,8 @@ import DiffFileComponent from '~/diffs/components/diff_file.vue'; import DiffFileHeaderComponent from '~/diffs/components/diff_file_header.vue'; import DiffContentComponent from '~/diffs/components/diff_content.vue'; +import eventHub from '~/diffs/event_hub'; + import { diffViewerModes, diffViewerErrors } from '~/ide/constants'; function changeViewer(store, index, { automaticallyCollapsed, manuallyCollapsed, name }) { @@ -138,6 +140,30 @@ describe('DiffFile', () => { }); describe('collapsing', () => { + describe('`mr:diffs:expandAllFiles` event', () => { + beforeEach(() => { + jest.spyOn(wrapper.vm, 'handleToggle').mockImplementation(() => {}); + }); + + it('performs the normal file toggle when the file is collapsed', async () => { + makeFileAutomaticallyCollapsed(store); + + await wrapper.vm.$nextTick(); + + eventHub.$emit('mr:diffs:expandAllFiles'); + + expect(wrapper.vm.handleToggle).toHaveBeenCalledTimes(1); + }); + + it('does nothing when the file is not collapsed', async () => { + eventHub.$emit('mr:diffs:expandAllFiles'); + + await wrapper.vm.$nextTick(); + + expect(wrapper.vm.handleToggle).not.toHaveBeenCalled(); + }); + }); + describe('user collapsed', () => { beforeEach(() => { makeFileManuallyCollapsed(store); -- GitLab From 7d7ab0a81ed4923655a2fc49a3ce02e3bba4f0eb Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 28 Oct 2020 13:30:43 -0600 Subject: [PATCH 2/2] Remove Vuex action for expanding all files The expandAllFiles action isn't used, so it can be deleted. Same with the mutation just for that action - EXPAND_ALL_FILES. --- app/assets/javascripts/diffs/store/actions.js | 4 ---- .../javascripts/diffs/store/mutation_types.js | 1 - .../javascripts/diffs/store/mutations.js | 11 ----------- spec/frontend/diffs/store/actions_spec.js | 18 ------------------ spec/frontend/diffs/store/mutations_spec.js | 15 --------------- 5 files changed, 49 deletions(-) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 322051cff05cbf..e4f79e76cadbf3 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -364,10 +364,6 @@ export const loadCollapsedDiff = ({ commit, getters, state }, file) => }); }); -export const expandAllFiles = ({ commit }) => { - commit(types.EXPAND_ALL_FILES); -}; - /** * Toggles the file discussions after user clicked on the toggle discussions button. * diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index 5dba2e9d10df51..19a9e65edc9151 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -13,7 +13,6 @@ export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS'; export const TOGGLE_LINE_HAS_FORM = 'TOGGLE_LINE_HAS_FORM'; export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES'; export const ADD_COLLAPSED_DIFFS = 'ADD_COLLAPSED_DIFFS'; -export const EXPAND_ALL_FILES = 'EXPAND_ALL_FILES'; export const RENDER_FILE = 'RENDER_FILE'; export const SET_LINE_DISCUSSIONS_FOR_FILE = 'SET_LINE_DISCUSSIONS_FOR_FILE'; export const REMOVE_LINE_DISCUSSIONS_FOR_FILE = 'REMOVE_LINE_DISCUSSIONS_FOR_FILE'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 5328845e4d9457..2aeecf6e9f534e 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -176,17 +176,6 @@ export default { Object.assign(selectedFile, { ...newFileData }); }, - [types.EXPAND_ALL_FILES](state) { - state.diffFiles.forEach(file => { - Object.assign(file, { - viewer: Object.assign(file.viewer, { - automaticallyCollapsed: false, - manuallyCollapsed: false, - }), - }); - }); - }, - [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) { const { latestDiff } = state; diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index d41c4ae44ebddd..47d92d4a868501 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -27,7 +27,6 @@ import { scrollToLineIfNeededInline, scrollToLineIfNeededParallel, loadCollapsedDiff, - expandAllFiles, toggleFileDiscussions, saveDiffDiscussion, setHighlightedRow, @@ -658,23 +657,6 @@ describe('DiffsStoreActions', () => { }); }); - describe('expandAllFiles', () => { - it('should change the collapsed prop from the diffFiles', done => { - testAction( - expandAllFiles, - null, - {}, - [ - { - type: types.EXPAND_ALL_FILES, - }, - ], - [], - done, - ); - }); - }); - describe('toggleFileDiscussions', () => { it('should dispatch collapseDiscussion when all discussions are expanded', () => { const getters = { diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index a84ad63c6956d4..c0645faf89e845 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -126,21 +126,6 @@ describe('DiffsStoreMutations', () => { }); }); - describe('EXPAND_ALL_FILES', () => { - it('should change the collapsed prop from diffFiles', () => { - const diffFile = { - viewer: { - automaticallyCollapsed: true, - }, - }; - const state = { expandAllFiles: true, diffFiles: [diffFile] }; - - mutations[types.EXPAND_ALL_FILES](state); - - expect(state.diffFiles[0].viewer.automaticallyCollapsed).toEqual(false); - }); - }); - describe('ADD_CONTEXT_LINES', () => { it('should call utils.addContextLines with proper params', () => { const options = { -- GitLab