-
Notifications
You must be signed in to change notification settings - Fork 319
fix(preview-sidebar): fix version not updating on small screens #3095
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
base: master
Are you sure you want to change the base?
fix(preview-sidebar): fix version not updating on small screens #3095
Conversation
@greathmaster, has the desktop change been approved by the relevant stakeholders? |
There was a problem hiding this 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() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
After