From 03d3117b7919de4c5e5a8a4233e0773d6d7dd609 Mon Sep 17 00:00:00 2001 From: Yixuan Qian Date: Tue, 3 Jan 2023 14:48:15 -0800 Subject: [PATCH 1/3] fix(preview-sidebar): fix preview versions behavior --- src/constants.js | 3 +++ src/elements/content-sidebar/Sidebar.js | 15 +++++++++++++-- src/elements/content-sidebar/SidebarPanels.js | 3 ++- .../content-sidebar/__tests__/Sidebar.test.js | 17 +++++++++++++++++ .../versions/VersionsSidebarContainer.js | 5 ----- .../__tests__/VersionsSidebarContainer.test.js | 11 ----------- 6 files changed, 35 insertions(+), 19 deletions(-) diff --git a/src/constants.js b/src/constants.js index df418ffafa..0e69ebc945 100644 --- a/src/constants.js +++ b/src/constants.js @@ -394,6 +394,9 @@ export const SIDEBAR_VIEW_METADATA: 'metadata' = 'metadata'; export const SIDEBAR_VIEW_ACTIVITY: 'activity' = 'activity'; export const SIDEBAR_VIEW_VERSIONS: 'versions' = 'versions'; +/* ------------------ Sidebar Path ---------------------- */ +export const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?'; + /* ------------------ HTTP Requests ---------------------- */ export const HTTP_GET: 'GET' = 'GET'; export const HTTP_POST: 'POST' = 'POST'; diff --git a/src/elements/content-sidebar/Sidebar.js b/src/elements/content-sidebar/Sidebar.js index 7ef6fa68e6..c101dffd4d 100644 --- a/src/elements/content-sidebar/Sidebar.js +++ b/src/elements/content-sidebar/Sidebar.js @@ -10,13 +10,14 @@ import flow from 'lodash/flow'; import getProp from 'lodash/get'; import noop from 'lodash/noop'; import uniqueid from 'lodash/uniqueId'; -import { withRouter } from 'react-router-dom'; +import { matchPath, withRouter } from 'react-router-dom'; import type { Location, RouterHistory } from 'react-router-dom'; import LoadingIndicator from '../../components/loading-indicator/LoadingIndicator'; import LocalStore from '../../utils/LocalStore'; import SidebarNav from './SidebarNav'; import SidebarPanels from './SidebarPanels'; import SidebarUtils from './SidebarUtils'; +import { SIDEBAR_PATH_VERSIONS } from '../../constants'; import { withCurrentUser } from '../common/current-user'; import { withFeatureConsumer } from '../common/feature-checking'; import type { FeatureConfig } from '../common/feature-checking'; @@ -97,7 +98,7 @@ class Sidebar extends React.Component { } componentDidUpdate(prevProps: Props): void { - const { fileId, history, location }: Props = this.props; + const { fileId, history, location, onVersionChange }: Props = this.props; const { fileId: prevFileId, location: prevLocation }: Props = prevProps; const { isDirty }: State = this.state; @@ -111,8 +112,18 @@ class Sidebar extends React.Component { this.setForcedByLocation(); this.setState({ isDirty: true }); } + + // Reset the current version id if the wrapping versions route is no longer active + if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) { + onVersionChange(null); + } } + getVersionsMatchPath = (location: Location) => { + const pathname = getProp(location, 'pathname', ''); + return matchPath(pathname, SIDEBAR_PATH_VERSIONS); + }; + getUrlPrefix = (pathname: string) => { const basePath = pathname.substring(1).split('/')[0]; return basePath; diff --git a/src/elements/content-sidebar/SidebarPanels.js b/src/elements/content-sidebar/SidebarPanels.js index 916ff3a9e0..582951f11d 100644 --- a/src/elements/content-sidebar/SidebarPanels.js +++ b/src/elements/content-sidebar/SidebarPanels.js @@ -18,6 +18,7 @@ import { ORIGIN_METADATA_SIDEBAR, ORIGIN_SKILLS_SIDEBAR, ORIGIN_VERSIONS_SIDEBAR, + SIDEBAR_PATH_VERSIONS, SIDEBAR_VIEW_ACTIVITY, SIDEBAR_VIEW_DETAILS, SIDEBAR_VIEW_METADATA, @@ -250,7 +251,7 @@ class SidebarPanels extends React.Component { )} {hasVersions && ( ( { wrapper.setProps({ location: { pathname: '/', state: { open: false } } }); expect(instance.isForced).toHaveBeenCalledWith(false); }); + + test('should reset the current version if the versions route is no longer active', () => { + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ location: { pathname: '/activity' }, onVersionChange }); + + wrapper.setProps({ location: { pathname: '/activity/versions/123', state: { open: true } } }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/activity/versions/123', state: { open: false } } }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/activity/versions/456', state: { open: true } } }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/details' } }); + expect(onVersionChange).lastCalledWith(null); + }); }); describe('handleVersionHistoryClick', () => { diff --git a/src/elements/content-sidebar/versions/VersionsSidebarContainer.js b/src/elements/content-sidebar/versions/VersionsSidebarContainer.js index 175a773d11..c8d7596a95 100644 --- a/src/elements/content-sidebar/versions/VersionsSidebarContainer.js +++ b/src/elements/content-sidebar/versions/VersionsSidebarContainer.js @@ -95,11 +95,6 @@ class VersionsSidebarContainer extends React.Component { } } - componentWillUnmount() { - // Reset the current version id since the wrapping route is no longer active - this.props.onVersionChange(null); - } - handleActionDelete = (versionId: string): Promise => { this.setState({ isLoading: true }); diff --git a/src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js b/src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js index 6d7f2c26d5..6178d9efad 100644 --- a/src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js +++ b/src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js @@ -58,17 +58,6 @@ describe('elements/content-sidebar/versions/VersionsSidebarContainer', () => { }); }); - describe('componentWillUnmount', () => { - test('should forward verison id reset to the parent component', () => { - const onVersionChange = jest.fn(); - const wrapper = getWrapper({ onVersionChange }); - - wrapper.instance().componentWillUnmount(); - - expect(onVersionChange).toBeCalledWith(null); - }); - }); - describe('componentDidMount', () => { test('should call onLoad after a successful fetchData() call', async () => { const onLoad = jest.fn(); From 9b2f2940237b29a6172601024dbcaaae5c1f82e4 Mon Sep 17 00:00:00 2001 From: Yixuan Qian Date: Thu, 10 Aug 2023 08:16:36 -0700 Subject: [PATCH 2/3] fix(preview-sidebar): move code to SidebarPanels --- src/constants.js | 3 -- src/elements/content-sidebar/Sidebar.js | 15 +----- src/elements/content-sidebar/SidebarPanels.js | 21 ++++++++- .../content-sidebar/__tests__/Sidebar.test.js | 17 ------- .../__tests__/SidebarPanels.test.js | 46 ++++++++++++++----- 5 files changed, 55 insertions(+), 47 deletions(-) diff --git a/src/constants.js b/src/constants.js index c3ef1bf1c4..e4653b9d37 100644 --- a/src/constants.js +++ b/src/constants.js @@ -396,9 +396,6 @@ export const SIDEBAR_VIEW_METADATA: 'metadata' = 'metadata'; export const SIDEBAR_VIEW_ACTIVITY: 'activity' = 'activity'; export const SIDEBAR_VIEW_VERSIONS: 'versions' = 'versions'; -/* ------------------ Sidebar Path ---------------------- */ -export const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?'; - /* ------------------ HTTP Requests ---------------------- */ export const HTTP_GET: 'GET' = 'GET'; export const HTTP_POST: 'POST' = 'POST'; diff --git a/src/elements/content-sidebar/Sidebar.js b/src/elements/content-sidebar/Sidebar.js index c101dffd4d..7ef6fa68e6 100644 --- a/src/elements/content-sidebar/Sidebar.js +++ b/src/elements/content-sidebar/Sidebar.js @@ -10,14 +10,13 @@ import flow from 'lodash/flow'; import getProp from 'lodash/get'; import noop from 'lodash/noop'; import uniqueid from 'lodash/uniqueId'; -import { matchPath, withRouter } from 'react-router-dom'; +import { withRouter } from 'react-router-dom'; import type { Location, RouterHistory } from 'react-router-dom'; import LoadingIndicator from '../../components/loading-indicator/LoadingIndicator'; import LocalStore from '../../utils/LocalStore'; import SidebarNav from './SidebarNav'; import SidebarPanels from './SidebarPanels'; import SidebarUtils from './SidebarUtils'; -import { SIDEBAR_PATH_VERSIONS } from '../../constants'; import { withCurrentUser } from '../common/current-user'; import { withFeatureConsumer } from '../common/feature-checking'; import type { FeatureConfig } from '../common/feature-checking'; @@ -98,7 +97,7 @@ class Sidebar extends React.Component { } componentDidUpdate(prevProps: Props): void { - const { fileId, history, location, onVersionChange }: Props = this.props; + const { fileId, history, location }: Props = this.props; const { fileId: prevFileId, location: prevLocation }: Props = prevProps; const { isDirty }: State = this.state; @@ -112,18 +111,8 @@ class Sidebar extends React.Component { this.setForcedByLocation(); this.setState({ isDirty: true }); } - - // Reset the current version id if the wrapping versions route is no longer active - if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) { - onVersionChange(null); - } } - getVersionsMatchPath = (location: Location) => { - const pathname = getProp(location, 'pathname', ''); - return matchPath(pathname, SIDEBAR_PATH_VERSIONS); - }; - getUrlPrefix = (pathname: string) => { const basePath = pathname.substring(1).split('/')[0]; return basePath; diff --git a/src/elements/content-sidebar/SidebarPanels.js b/src/elements/content-sidebar/SidebarPanels.js index 582951f11d..1529d08e96 100644 --- a/src/elements/content-sidebar/SidebarPanels.js +++ b/src/elements/content-sidebar/SidebarPanels.js @@ -6,7 +6,7 @@ import * as React from 'react'; import flow from 'lodash/flow'; -import { Redirect, Route, Switch } from 'react-router-dom'; +import { matchPath, Redirect, Route, Switch, type Location } from 'react-router-dom'; import SidebarUtils from './SidebarUtils'; import withSidebarAnnotations from './withSidebarAnnotations'; import { withAnnotatorContext } from '../common/annotator-context'; @@ -18,7 +18,6 @@ import { ORIGIN_METADATA_SIDEBAR, ORIGIN_SKILLS_SIDEBAR, ORIGIN_VERSIONS_SIDEBAR, - SIDEBAR_PATH_VERSIONS, SIDEBAR_VIEW_ACTIVITY, SIDEBAR_VIEW_DETAILS, SIDEBAR_VIEW_METADATA, @@ -48,6 +47,7 @@ type Props = { hasSkills: boolean, hasVersions: boolean, isOpen: boolean, + location: Location, metadataSidebarProps: MetadataSidebarProps, onAnnotationSelect?: Function, onVersionChange?: Function, @@ -88,6 +88,8 @@ const LoadableVersionsSidebar = SidebarUtils.getAsyncSidebarContent( MARK_NAME_JS_LOADING_VERSIONS, ); +const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?'; + class SidebarPanels extends React.Component { activitySidebar: ElementRefType = React.createRef(); @@ -103,6 +105,21 @@ class SidebarPanels extends React.Component { this.setState({ isInitialized: true }); } + componentDidUpdate(prevProps: Props): void { + const { location, onVersionChange }: Props = this.props; + const { location: prevLocation }: Props = prevProps; + + // Reset the current version id if the wrapping versions route is no longer active + if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) { + onVersionChange(null); + } + } + + getVersionsMatchPath = (location: Location) => { + const { pathname } = location; + return matchPath(pathname, SIDEBAR_PATH_VERSIONS); + }; + /** * Refreshes the contents of the active sidebar * @returns {void} diff --git a/src/elements/content-sidebar/__tests__/Sidebar.test.js b/src/elements/content-sidebar/__tests__/Sidebar.test.js index 9137b1e585..eae3aad4a4 100644 --- a/src/elements/content-sidebar/__tests__/Sidebar.test.js +++ b/src/elements/content-sidebar/__tests__/Sidebar.test.js @@ -74,23 +74,6 @@ describe('elements/content-sidebar/Sidebar', () => { wrapper.setProps({ location: { pathname: '/', state: { open: false } } }); expect(instance.isForced).toHaveBeenCalledWith(false); }); - - test('should reset the current version if the versions route is no longer active', () => { - const onVersionChange = jest.fn(); - const wrapper = getWrapper({ location: { pathname: '/activity' }, onVersionChange }); - - wrapper.setProps({ location: { pathname: '/activity/versions/123', state: { open: true } } }); - expect(onVersionChange).not.toBeCalled(); - - wrapper.setProps({ location: { pathname: '/activity/versions/123', state: { open: false } } }); - expect(onVersionChange).not.toBeCalled(); - - wrapper.setProps({ location: { pathname: '/activity/versions/456', state: { open: true } } }); - expect(onVersionChange).not.toBeCalled(); - - wrapper.setProps({ location: { pathname: '/details' } }); - expect(onVersionChange).lastCalledWith(null); - }); }); describe('handleVersionHistoryClick', () => { diff --git a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js index 2fd0030530..cb5e83c013 100644 --- a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js +++ b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js @@ -11,18 +11,23 @@ jest.mock('../SidebarUtils'); describe('elements/content-sidebar/SidebarPanels', () => { const getWrapper = ({ path = '/', ...rest } = {}) => mount( - - - , + , + { + wrappingComponent: MemoryRouter, + wrappingComponentProps: { + initialEntries: [path], + keyLength: 0, + }, + }, ); describe('render', () => { @@ -144,4 +149,21 @@ describe('elements/content-sidebar/SidebarPanels', () => { expect(instance.versionsSidebar.current.refresh).toHaveBeenCalledWith(); }); }); + + test('should reset the current version if the versions route is no longer active', () => { + const onVersionChange = jest.fn(); + const wrapper = getWrapper({ location: { pathname: '/activity' }, onVersionChange }); + + wrapper.setProps({ location: { pathname: '/activity/versions/123' }, isOpen: true }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/activity/versions/123' }, isOpen: false }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/activity/versions/456' }, isOpen: true }); + expect(onVersionChange).not.toBeCalled(); + + wrapper.setProps({ location: { pathname: '/details' } }); + expect(onVersionChange).lastCalledWith(null); + }); }); From 201c658b78698e5404945445964ff596c8744fda Mon Sep 17 00:00:00 2001 From: Yixuan Qian Date: Thu, 10 Aug 2023 09:21:56 -0700 Subject: [PATCH 3/3] fix(preview-sidebar): update tests; remove types --- src/elements/content-sidebar/SidebarPanels.js | 4 +- .../__tests__/SidebarPanels.test.js | 39 +++++++++++++------ 2 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/elements/content-sidebar/SidebarPanels.js b/src/elements/content-sidebar/SidebarPanels.js index 1529d08e96..4d2b0359f2 100644 --- a/src/elements/content-sidebar/SidebarPanels.js +++ b/src/elements/content-sidebar/SidebarPanels.js @@ -106,8 +106,8 @@ class SidebarPanels extends React.Component { } componentDidUpdate(prevProps: Props): void { - const { location, onVersionChange }: Props = this.props; - const { location: prevLocation }: Props = prevProps; + const { location, onVersionChange } = this.props; + const { location: prevLocation } = prevProps; // Reset the current version id if the wrapping versions route is no longer active if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) { diff --git a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js index cb5e83c013..579fbf6f17 100644 --- a/src/elements/content-sidebar/__tests__/SidebarPanels.test.js +++ b/src/elements/content-sidebar/__tests__/SidebarPanels.test.js @@ -150,20 +150,37 @@ describe('elements/content-sidebar/SidebarPanels', () => { }); }); - test('should reset the current version if the versions route is no longer active', () => { + describe('componentDidUpdate', () => { const onVersionChange = jest.fn(); - const wrapper = getWrapper({ location: { pathname: '/activity' }, onVersionChange }); - wrapper.setProps({ location: { pathname: '/activity/versions/123' }, isOpen: true }); - expect(onVersionChange).not.toBeCalled(); - - wrapper.setProps({ location: { pathname: '/activity/versions/123' }, isOpen: false }); - expect(onVersionChange).not.toBeCalled(); + test.each([ + ['/activity/versions/123', '/activity/versions/456'], + ['/activity/versions/123', '/details/versions/456'], + ['/activity/versions', '/activity/versions/123'], + ['/activity/versions', '/details/versions'], + ])('should not reset the current version if the versions route is still active', (prevPathname, pathname) => { + const wrapper = getWrapper({ location: { pathname: prevPathname }, onVersionChange }); + wrapper.setProps({ location: { pathname } }); + expect(onVersionChange).not.toBeCalled(); + }); - wrapper.setProps({ location: { pathname: '/activity/versions/456' }, isOpen: true }); - expect(onVersionChange).not.toBeCalled(); + test.each([true, false])('should not reset the current version if the sidebar is toggled', isOpen => { + const wrapper = getWrapper({ isOpen, location: { pathname: '/details/versions/123' }, onVersionChange }); + wrapper.setProps({ isOpen: !isOpen }); + expect(onVersionChange).not.toBeCalled(); + }); - wrapper.setProps({ location: { pathname: '/details' } }); - expect(onVersionChange).lastCalledWith(null); + test.each([ + ['/activity/versions/123', '/metadata'], + ['/activity/versions/123', '/activity'], + ['/activity/versions', '/metadata'], + ['/details/versions/123', '/metadata'], + ['/details/versions/123', '/details'], + ['/details/versions', '/metadata'], + ])('should reset the current version if the versions route is no longer active', (prevPathname, pathname) => { + const wrapper = getWrapper({ location: { pathname: prevPathname }, onVersionChange }); + wrapper.setProps({ location: { pathname } }); + expect(onVersionChange).toBeCalledWith(null); + }); }); });