Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(preview-sidebar): fix version not updating on small screens #3095

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(preview-sidebar): fix version not updating on small screens #3095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/elements/content-sidebar/versions/VersionsSidebarContainer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,11 +95,6 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
}
}

componentWillUnmount() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey so are there any possible side effects of removing this code completely. If we can remove it then it looks like it may have been unnecessary code in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code was necessary to support updating to the latest preview version after closing the sidebar.

The change updates the behavior to remain on the version that was selected after closing the sidebar.

So it was needed before, but with the change in behavior it is not needed anymore. I don't believe it will have any side effects.

@jstoffan Do you see any potential side effects with this change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also means that switching to other sidebars, like "Details" or "Metadata", will not revert to the current version preview. The only mechanism to do so will be the button in the header.

Have we heard back from the relevant stakeholders that this change is acceptable even on larger screen sizes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have approval for the version behavior change, ie to show the previous version after closing the right sidebar on large screens. However, let me circle back after checking about the "Details" and "Metadata" experiences.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@greathmaster, understood. I would think the Next/Previous buttons to navigate forward and backward through previews in a folder may also be affected.

// Reset the current version id since the wrapping route is no longer active
this.props.onVersionChange(null);
}

handleActionDelete = (versionId: string): Promise<void> => {
this.setState({ isLoading: true });

Expand Down
11 changes: 0 additions & 11 deletions src/elements/content-sidebar/versions/__tests__/VersionsSidebarContainer.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down