Sidebar on window resize #2899
Sidebar on window resize #2899
Conversation
|
@esanzgar I like the behavior on this branch! I know that it's technically inconsistent: closing the sidebar when resizing an "HTML" document but leaving it as-is when in "PDF mode," but it feels intuitively correct to me. I have a hunch users may have become accustomed to the sidebar not closing on PDF side-by-side mode, and, since the sidebar is not overlapping PDF content while the window is resizing (unless you make the window really tiny, which is not likely a common use case; heck, resizing at all isn't terribly common when you think about it), leaving it open feels correct. Getting it out of the way on the HTML view—when content in the underlying page often does stretch all the way to the edges of the screen—feels good. A heads up that we will almost assuredly need to re-evaluate this when we implement HTML side-by-side mode (you can see a POC on this here: #2809. There's not an issue tracking the overall feature yet; it's a "note" in the product backlog. When K is back from her time out, I'll see if we can track this more concretely in an issue. But I do know it's something that is in the future). |
5bcdeed to
62c82ad
Compare
Currently, if the client's sidebar is opened it doesn't behave graciously when the browser window is resized. Sometimes the sidebar appears floating on the middle of the window. We discussed several alternative solutions: when the sidebar is opened and the window is resized, then... 1. close the sidebar and the user opens it manually 2. maintain the sidebar open, but scale it properly 3. close the sidebar and open it after X milliseconds of the last resize event 4. close the sidebar if the width of the window is reduced, but maintain it open if the window's width is increased We implemented solution #1. However, this issue is still visible on the PDF-sidebar. It would be great if the both sidebars (PDF and non-PDF) have the same behaviour. In addition, I added a mechanism to register and unregister events, to avoid resource leaks.
62c82ad to
126384d
Compare
There was a problem hiding this comment.
Eduardo suggested in Slack that I take a look at this as well. Here are my initial thoughts:
As a user, I think I'd prefer the sidebar to remain in whatever state it was in when the window resizes. Changing the state of the sidebar when the window is resized also has an unfortunate effect in macOS of seemingly making the resizing feel more laggy. I suspect this may not happen as much (or maybe at all) on Windows / Linux because window resizing works differently there.
As noted in the PR description, the original issue of the sidebar becoming detached still affects the PDF sidebar. It seems like there is a problem that we are papering over here that we need to fix. It looks to me like the resizing policy changes at some point when the window becomes narrower and the sidebar is not repositioned to account for that if it is already open?
Finally, as Lyza noted, we are planning to look into introducing something similar to the PDF side-by-side mode for HTML documents soon. If we do that, then I expect the current issue that occurs on PDFs with this branch will also occur with many HTML documents.
| @@ -20,7 +20,7 @@ const MIN_PDF_WIDTH = 680; | |||
|
|
|||
| export default class PdfSidebar extends Sidebar { | |||
| constructor(element, config) { | |||
| super(element, { ...defaultConfig, ...config }); | |||
| super(element, { ...defaultConfig, ...config }, false); | |||
There was a problem hiding this comment.
It's not immediately clear from looking at this code what false. I would suggest that all configuration options should go into the existing config argument where we can give it a useful name and more easily add new options.
A more significant issue though is that there is an existing issue with the PDF sidebar where it becomes "detached" when the window size is reduced below a certain size. I think this is the issue that you mentioned in the PR description?
| @@ -215,11 +254,11 @@ export default class Sidebar extends Guest { | |||
| const toggleButton = this.toolbar.sidebarToggleButton; | |||
| if (toggleButton) { | |||
| // Prevent any default gestures on the handle. | |||
| toggleButton.addEventListener('touchmove', e => e.preventDefault()); | |||
| this._registerEvent(toggleButton, 'touchmove', e => e.preventDefault()); | |||
There was a problem hiding this comment.
The toggle button is part of an element that is owned by the sidebar. Since the sidebar frame is already removed by destroy(), we don't need to explicitly unsubscribe when the sidebar is destroyed. It will happen automatically as a side effect of removing the DOM element.
There was a problem hiding this comment.
You are correct. However, it is a good idea to always remove all the events that have been added as a defensive programming technique.
An example of why this is important: the sidebar (and pdf-sidebar) had an events attached to the window. Even when this element was destroy there was a dangling listener that contained a link to the sidebar component.
| * eventType: ArgAddEventListener[0], | ||
| * listener:ArgAddEventListener[1] | ||
| * }} RegisteredListener | ||
| */ |
There was a problem hiding this comment.
This works, although I probably would have gone with some types that were a bit more generic and simpler. Perhaps something like:
/**
* @typedef {{
* target: EventTarget,
* eventType: string,
* listener: (e: Event) => void,
* }} RegisteredListener
*/I notice that we have similar code in guest.js. It would probably be a good idea to extract a utility in future for this.
|
Closed in favour of #2937 which maintains the sidebar in the current state (open or closed). |
Hide sidebar when browser window is resized
Currently, if the client's sidebar is opened it doesn't behave
graciously when the browser window is resized. Sometimes the sidebar
appears floating on the middle of the window.
We discussed several alternative solutions. When the sidebar is opened and the window is resized, then:
event
maintain it open if the window's width is increased
We implemented solution #1. However, this issue is still visible on the
PDF-sidebar. It would be great if the both sidebars (PDF and non-PDF)
have the same behaviour.
In addition, I added a mechanism to register and unregister events, to
avoid resource leaks.