fix(preview-sidebar): fix preview versions behavior #3203
Conversation
Machoper
force-pushed
the
fix-preview-versions-behavior
branch
from
d6fe805 to
03d3117
Compare
There was a problem hiding this comment.
looks good from my testing
one thing to note: there seems to be an existing bug where if you go to an older file version and click the Next/Previous buttons, the version number of the previous file is still in the URL for the new file. but this is not a regression from this change
src/constants.js
Outdated
| @@ -396,6 +396,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?'; | |||
There was a problem hiding this comment.
Can we store this in a module within the content-sidebar directory?
| } | ||
|
|
||
| getVersionsMatchPath = (location: Location) => { | ||
| const pathname = getProp(location, 'pathname', ''); |
There was a problem hiding this comment.
Do we need getProp here? I think location is always defined.
| @@ -111,8 +112,18 @@ class Sidebar extends React.Component<Props, State> { | |||
| 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)) { | |||
There was a problem hiding this comment.
ContentSidebar already does a lot. I'm wondering if this would be better suited to live in SidebarPanels, since that's where we manage all of this already.
| expect(onVersionChange).not.toBeCalled(); | ||
|
|
||
| wrapper.setProps({ location: { pathname: '/details' } }); | ||
| expect(onVersionChange).lastCalledWith(null); |
There was a problem hiding this comment.
Would .toBeCalledWith(null) work here or is the number and order of calls important?
There was a problem hiding this comment.
It's from an earlier attempt to run a few assertions in series and I forgot to update it afterwards.
I just rearranged the tests and change it to .toBeCalledWith(null).
| const onVersionChange = jest.fn(); | ||
| const wrapper = getWrapper({ location: { pathname: '/activity' }, onVersionChange }); | ||
|
|
||
| wrapper.setProps({ location: { pathname: '/activity/versions/123' }, isOpen: true }); |
There was a problem hiding this comment.
Can we add assertions for /activity/versions and /details/versions, as well?
| isOpen | ||
| {...rest} | ||
| />, | ||
| { |
There was a problem hiding this comment.
Curious, what was the reason for this change?
There was a problem hiding this comment.
setProps() can only be called on the root, which was MemoryRouter in the previous setup. So, this is more of a workaround to call setProps() on SidebarPanels instead and trigger componentDidUpdate().
| @@ -102,6 +105,21 @@ class SidebarPanels extends React.Component<Props, State> { | |||
| this.setState({ isInitialized: true }); | |||
| } | |||
|
|
|||
| componentDidUpdate(prevProps: Props): void { | |||
| const { location, onVersionChange }: Props = this.props; | |||
| const { location: prevLocation }: Props = prevProps; | |||
There was a problem hiding this comment.
Are the : Props explicit types needed here? I would think the type would be inferred from the variables.
There was a problem hiding this comment.
No, they are not needed. I copy-pasted from Sidebar.js. Deleted!
| @@ -144,4 +149,38 @@ describe('elements/content-sidebar/SidebarPanels', () => { | |||
| expect(instance.versionsSidebar.current.refresh).toHaveBeenCalledWith(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('componentDidUpdate', () => { | |||
| const onVersionChange = jest.fn(); | |||
There was a problem hiding this comment.
just to confirm, the call counts are reset between the tests right?
There was a problem hiding this comment.
I think so. We have clearMocks: true in jest.config.js
Overview
This PR is built on #3095, so I'm borrowing the same description with a few small changes:
Currently, if a version is selected in the Preview sidebar and the Preview sidebar is closed, it reverts to the latest version of the document.
This creates problems for small screens where the Preview sidebar is moved to the bottom and becomes a vertical sliding drawer. When an old version is selected and the drawer is closed, a user cannot view the old version because it reverts to the latest version of the document.
This PR fixes the issue by adjusting the logic to reset the current version only when the user navigates away from version history.
Note this would also affect the regular desktop experience. If a user closes the Preview Sidebar after selecting the version, it would remain on that version instead of changing back to the latest version (approved by relevant stakeholders). This would not affect the current behavior when switching to other sidebars like "Details" or "Metadata".
Demo
Before
small screen
large screen
After
small screen
large screen