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.

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

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

Conversation

greathmaster
Copy link
Contributor

Overview

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 removing the version reset performed when the drawer is closed.

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.

Demo

Before

Preview-Drawer-Before

After

Preview-Drawer-After

@jstoffan
Copy link
Contributor

@greathmaster, has the desktop change been approved by the relevant stakeholders?

Copy link
Contributor Author

@greathmaster greathmaster left a comment

Choose a reason for hiding this comment

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

I've asked the relevant stakeholders and they are okay with the the behavior change on desktop experiences.

@@ -95,11 +95,6 @@ class VersionsSidebarContainer extends React.Component<Props, State> {
}
}

componentWillUnmount() {

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants