From 75ca86e7a81f0d086c7d0c55f350b4c122f4bf1e Mon Sep 17 00:00:00 2001
From: Tom Quirk <tquirk@gitlab.com>
Date: Thu, 22 Oct 2020 07:37:34 +0000
Subject: [PATCH] Move fullscreen logic from router to component
This scopes complexity to the component for better
maintainability
---
.../design_management/pages/design/index.vue | 19 ++-
.../design_management/router/index.js | 17 ---
.../pages/design/index_spec.js | 110 +++++++++++-------
.../design_management/pages/index_spec.js | 8 --
4 files changed, 87 insertions(+), 67 deletions(-)
diff --git a/app/assets/javascripts/design_management/pages/design/index.vue b/app/assets/javascripts/design_management/pages/design/index.vue
index 6a96b06dcd89..642d51fbc7a6 100644
--- a/app/assets/javascripts/design_management/pages/design/index.vue
+++ b/app/assets/javascripts/design_management/pages/design/index.vue
@@ -21,6 +21,7 @@ import {
updateImageDiffNoteOptimisticResponse,
toDiffNoteGid,
extractDesignNoteId,
+ getPageLayoutElement,
} from '../../utils/design_management_utils';
import {
updateStoreAfterAddImageDiffNote,
@@ -38,7 +39,7 @@ import {
} from '../../utils/error_messages';
import { trackDesignDetailView } from '../../utils/tracking';
import { DESIGNS_ROUTE_NAME } from '../../router/constants';
-import { ACTIVE_DISCUSSION_SOURCE_TYPES } from '../../constants';
+import { ACTIVE_DISCUSSION_SOURCE_TYPES, DESIGN_DETAIL_LAYOUT_CLASSLIST } from '../../constants';
const DEFAULT_SCALE = 1;
@@ -300,6 +301,22 @@ export default {
this.resolvedDiscussionsExpanded = !this.resolvedDiscussionsExpanded;
},
},
+ beforeRouteEnter(to, from, next) {
+ const pageEl = getPageLayoutElement();
+ if (pageEl) {
+ pageEl.classList.add(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
+ }
+
+ next();
+ },
+ beforeRouteLeave(to, from, next) {
+ const pageEl = getPageLayoutElement();
+ if (pageEl) {
+ pageEl.classList.remove(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
+ }
+
+ next();
+ },
createImageDiffNoteMutation,
DESIGNS_ROUTE_NAME,
};
diff --git a/app/assets/javascripts/design_management/router/index.js b/app/assets/javascripts/design_management/router/index.js
index cbeb2f7ce424..12692612bbcd 100644
--- a/app/assets/javascripts/design_management/router/index.js
+++ b/app/assets/javascripts/design_management/router/index.js
@@ -1,9 +1,6 @@
import Vue from 'vue';
import VueRouter from 'vue-router';
import routes from './routes';
-import { DESIGN_ROUTE_NAME } from './constants';
-import { getPageLayoutElement } from '~/design_management/utils/design_management_utils';
-import { DESIGN_DETAIL_LAYOUT_CLASSLIST } from '../constants';
Vue.use(VueRouter);
@@ -13,20 +10,6 @@ export default function createRouter(base) {
mode: 'history',
routes,
});
- const pageEl = getPageLayoutElement();
-
- router.beforeEach(({ name }, _, next) => {
- // apply a fullscreen layout style in Design View (a.k.a design detail)
- if (pageEl) {
- if (name === DESIGN_ROUTE_NAME) {
- pageEl.classList.add(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
- } else {
- pageEl.classList.remove(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
- }
- }
-
- next();
- });
return router;
}
diff --git a/spec/frontend/design_management/pages/design/index_spec.js b/spec/frontend/design_management/pages/design/index_spec.js
index d9f7146d2586..d0c14c4c7eb1 100644
--- a/spec/frontend/design_management/pages/design/index_spec.js
+++ b/spec/frontend/design_management/pages/design/index_spec.js
@@ -24,7 +24,13 @@ import mockAllVersions from '../../mock_data/all_versions';
jest.mock('~/flash');
const focusInput = jest.fn();
-
+const mutate = jest.fn().mockResolvedValue();
+const mockPageLayoutElement = {
+ classList: {
+ add: jest.fn(),
+ remove: jest.fn(),
+ },
+};
const DesignReplyForm = {
template: '<div><textarea ref="textarea"></textarea></div>',
methods: {
@@ -37,6 +43,32 @@ const mockDesignNoDiscussions = {
nodes: [],
},
};
+const newComment = 'new comment';
+const annotationCoordinates = {
+ x: 10,
+ y: 10,
+ width: 100,
+ height: 100,
+};
+const createDiscussionMutationVariables = {
+ mutation: createImageDiffNoteMutation,
+ update: expect.anything(),
+ variables: {
+ input: {
+ body: newComment,
+ noteableId: design.id,
+ position: {
+ headSha: 'headSha',
+ baseSha: 'baseSha',
+ startSha: 'startSha',
+ paths: {
+ newPath: 'full-design-path',
+ },
+ ...annotationCoordinates,
+ },
+ },
+ },
+};
const localVue = createLocalVue();
localVue.use(VueRouter);
@@ -45,35 +77,6 @@ describe('Design management design index page', () => {
let wrapper;
let router;
- const newComment = 'new comment';
- const annotationCoordinates = {
- x: 10,
- y: 10,
- width: 100,
- height: 100,
- };
- const createDiscussionMutationVariables = {
- mutation: createImageDiffNoteMutation,
- update: expect.anything(),
- variables: {
- input: {
- body: newComment,
- noteableId: design.id,
- position: {
- headSha: 'headSha',
- baseSha: 'baseSha',
- startSha: 'startSha',
- paths: {
- newPath: 'full-design-path',
- },
- ...annotationCoordinates,
- },
- },
- },
- };
-
- const mutate = jest.fn().mockResolvedValue();
-
const findDiscussionForm = () => wrapper.find(DesignReplyForm);
const findSidebar = () => wrapper.find(DesignSidebar);
const findDesignPresentation = () => wrapper.find(DesignPresentation);
@@ -122,19 +125,44 @@ describe('Design management design index page', () => {
wrapper.destroy();
});
- describe('when navigating', () => {
- it('applies fullscreen layout', () => {
- const mockEl = {
- classList: {
- add: jest.fn(),
- remove: jest.fn(),
- },
- };
- jest.spyOn(utils, 'getPageLayoutElement').mockReturnValue(mockEl);
+ describe('when navigating to component', () => {
+ it('applies fullscreen layout class', () => {
+ jest.spyOn(utils, 'getPageLayoutElement').mockReturnValue(mockPageLayoutElement);
createComponent({ loading: true });
- expect(mockEl.classList.add).toHaveBeenCalledTimes(1);
- expect(mockEl.classList.add).toHaveBeenCalledWith(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
+ expect(mockPageLayoutElement.classList.add).toHaveBeenCalledTimes(1);
+ expect(mockPageLayoutElement.classList.add).toHaveBeenCalledWith(
+ ...DESIGN_DETAIL_LAYOUT_CLASSLIST,
+ );
+ });
+ });
+
+ describe('when navigating within the component', () => {
+ it('`scale` prop of DesignPresentation component is 1', async () => {
+ jest.spyOn(utils, 'getPageLayoutElement').mockReturnValue(mockPageLayoutElement);
+ createComponent({ loading: false }, { data: { design, scale: 2 } });
+
+ await wrapper.vm.$nextTick();
+ expect(findDesignPresentation().props('scale')).toBe(2);
+
+ DesignIndex.beforeRouteUpdate.call(wrapper.vm, {}, {}, jest.fn());
+ await wrapper.vm.$nextTick();
+
+ expect(findDesignPresentation().props('scale')).toBe(1);
+ });
+ });
+
+ describe('when navigating away from component', () => {
+ it('removes fullscreen layout class', async () => {
+ jest.spyOn(utils, 'getPageLayoutElement').mockReturnValue(mockPageLayoutElement);
+ createComponent({ loading: true });
+
+ wrapper.vm.$options.beforeRouteLeave[0].call(wrapper.vm, {}, {}, jest.fn());
+
+ expect(mockPageLayoutElement.classList.remove).toHaveBeenCalledTimes(1);
+ expect(mockPageLayoutElement.classList.remove).toHaveBeenCalledWith(
+ ...DESIGN_DETAIL_LAYOUT_CLASSLIST,
+ );
});
});
diff --git a/spec/frontend/design_management/pages/index_spec.js b/spec/frontend/design_management/pages/index_spec.js
index 27a91b114489..279403789f12 100644
--- a/spec/frontend/design_management/pages/index_spec.js
+++ b/spec/frontend/design_management/pages/index_spec.js
@@ -19,7 +19,6 @@ import {
import { deprecatedCreateFlash as createFlash } from '~/flash';
import createRouter from '~/design_management/router';
import * as utils from '~/design_management/utils/design_management_utils';
-import { DESIGN_DETAIL_LAYOUT_CLASSLIST } from '~/design_management/constants';
import {
designListQueryResponse,
designUploadMutationCreatedResponse,
@@ -682,13 +681,6 @@ describe('Design management index page', () => {
});
describe('when navigating', () => {
- it('ensures fullscreen layout is not applied', () => {
- createComponent({ loading: true });
-
- expect(mockPageEl.classList.remove).toHaveBeenCalledTimes(1);
- expect(mockPageEl.classList.remove).toHaveBeenCalledWith(...DESIGN_DETAIL_LAYOUT_CLASSLIST);
- });
-
it('should trigger a scrollIntoView method if designs route is detected', () => {
router.replace({
path: '/designs',
--
GitLab