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

Sidebar on window resize #2899

Closed
wants to merge 1 commit into from
Closed

Sidebar on window resize #2899

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: 3 additions & 2 deletions src/annotator/pdf-sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

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?

PDF sidebar narrow


this._lastSidebarLayoutState = {
expanded: false,
Expand All @@ -36,7 +36,8 @@ export default class PdfSidebar extends Sidebar {
this.sideBySideActive = false;

this.subscribe('sidebarLayoutChanged', state => this.fitSideBySide(state));
this.window.addEventListener('resize', () => this.fitSideBySide());

this._registerEvent(window, 'resize', () => this.fitSideBySide());
}

/**
Expand Down
60 changes: 54 additions & 6 deletions src/annotator/sidebar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ import { ToolbarController } from './toolbar';
* @prop {number} height
*/

/**
* @typedef {Window|HTMLElement} WindowOrHTMLElement
* @typedef {Parameters<WindowOrHTMLElement["addEventListener"]>} ArgAddEventListener
* @typedef {{
* object: WindowOrHTMLElement,
* eventType: ArgAddEventListener[0],
* listener:ArgAddEventListener[1]
* }} RegisteredListener
*/
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@esanzgar esanzgar Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I simplified it.


// Minimum width to which the frame can be resized.
const MIN_RESIZE = 280;

Expand Down Expand Up @@ -53,7 +63,11 @@ function createSidebarIframe(config) {
* as well as the adjacent controls.
*/
export default class Sidebar extends Guest {
constructor(element, config) {
/**
* @param {HTMLElement} element
* @param {Record<string, any>} config
*/
constructor(element, config, hideOnResize = true) {
if (config.theme === 'clean' || config.externalContainerSelector) {
delete config.pluginClasses.BucketBar;
}
Expand Down Expand Up @@ -87,6 +101,9 @@ export default class Sidebar extends Guest {

super(element, { ...defaultConfig, ...config });

/** @type {RegisteredListener[]} */
this.registeredListeners = [];

this.externalFrame = externalFrame;
this.frame = frame;
(frame || externalFrame).appendChild(sidebarFrame);
Expand Down Expand Up @@ -117,7 +134,7 @@ export default class Sidebar extends Guest {
}

if (this.plugins.BucketBar) {
this.plugins.BucketBar.element.addEventListener('click', () =>
this._registerEvent(this.plugins.BucketBar.element, 'click', () =>
this.show()
);
}
Expand All @@ -141,6 +158,10 @@ export default class Sidebar extends Guest {
this.toolbarWidth = 0;
}

if (hideOnResize) {
this._registerEvent(window, 'resize', () => this._onResize());
}

this._gestureState = {
// Initial position at the start of a drag/pan resize event (in pixels).
initial: /** @type {number|null} */ (null),
Expand Down Expand Up @@ -169,11 +190,29 @@ export default class Sidebar extends Guest {
}

destroy() {
this._unregisterEvents();
this._hammerManager?.destroy();
this.frame?.remove();
super.destroy();
}

/**
* @param {WindowOrHTMLElement} object
* @param {ArgAddEventListener[0]} eventType
* @param {ArgAddEventListener[1]} listener
*/
_registerEvent(object, eventType, listener) {
object.addEventListener(eventType, listener);
this.registeredListeners.push({ object, eventType, listener });
}

_unregisterEvents() {
this.registeredListeners.forEach(({ object, eventType, listener }) => {
object.removeEventListener(eventType, listener);
});
this.registeredListeners = [];
}

_setupSidebarEvents() {
annotationCounts(document.body, this.crossframe);
sidebarTrigger(document.body, () => this.show());
Expand Down Expand Up @@ -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());
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


this._hammerManager = new Hammer.Manager(toggleButton)
// eslint-disable-next-line no-restricted-properties
.on('panstart panend panleft panright', this._onPan.bind(this));
this._hammerManager = new Hammer.Manager(
toggleButton
).on('panstart panend panleft panright', () => this._onPan());
this._hammerManager.add(
new Hammer.Pan({ direction: Hammer.DIRECTION_HORIZONTAL })
);
Expand Down Expand Up @@ -308,6 +347,15 @@ export default class Sidebar extends Guest {
this.publish('sidebarLayoutChanged', [layoutState]);
}

/**
* Minimize the sidebar on window resize event
*/
_onResize() {
if (this.toolbar.sidebarOpen === true) {
this.hide();
}
}

_onPan(event) {
const frame = this.frame;
if (!frame) {
Expand Down
48 changes: 39 additions & 9 deletions src/annotator/test/sidebar-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ describe('Sidebar', () => {

describe('toolbar buttons', () => {
it('shows or hides sidebar when toolbar button is clicked', () => {
const sidebar = createSidebar({});
const sidebar = createSidebar();
sinon.stub(sidebar, 'show');
sinon.stub(sidebar, 'hide');

Expand All @@ -186,7 +186,7 @@ describe('Sidebar', () => {
});

it('shows or hides highlights when toolbar button is clicked', () => {
const sidebar = createSidebar({});
const sidebar = createSidebar();
sinon.stub(sidebar, 'setAllVisibleHighlights');

FakeToolbarController.args[0][1].setHighlightsVisible(true);
Expand All @@ -198,7 +198,7 @@ describe('Sidebar', () => {
});

it('creates an annotation when toolbar button is clicked', () => {
const sidebar = createSidebar({});
const sidebar = createSidebar();
sinon.stub(sidebar, 'createAnnotation');

FakeToolbarController.args[0][1].createAnnotation();
Expand Down Expand Up @@ -312,7 +312,7 @@ describe('Sidebar', () => {
});

it('does not crash if there is no services', () => {
createSidebar({}); // No config.services
createSidebar(); // No config.services
emitEvent(events.LOGIN_REQUESTED);
});

Expand Down Expand Up @@ -372,7 +372,7 @@ describe('Sidebar', () => {
let sidebar;

beforeEach(() => {
sidebar = createSidebar({});
sidebar = createSidebar();
});

describe('panstart event', () => {
Expand Down Expand Up @@ -469,7 +469,7 @@ describe('Sidebar', () => {
});

it('does not show the sidebar if not configured to.', () => {
const sidebar = createSidebar({});
const sidebar = createSidebar();
const show = sandbox.stub(sidebar, 'show');
sidebar.publish('panelReady');
assert.notCalled(show);
Expand All @@ -480,7 +480,7 @@ describe('Sidebar', () => {
let sidebar;

beforeEach(() => {
sidebar = createSidebar({});
sidebar = createSidebar();
});

it('the sidebar is destroyed and the frame is detached', () => {
Expand Down Expand Up @@ -534,7 +534,7 @@ describe('Sidebar', () => {

describe('#setAllVisibleHighlights', () =>
it('sets the state through crossframe and emits', () => {
const sidebar = createSidebar({});
const sidebar = createSidebar();
sidebar.setAllVisibleHighlights(true);
assert.calledWith(fakeCrossFrame.call, 'setVisibleHighlights', true);
}));
Expand All @@ -545,10 +545,40 @@ describe('Sidebar', () => {
});

it('shows toolbar controls when using the default theme', () => {
createSidebar({});
createSidebar();
assert.equal(fakeToolbar.useMinimalControls, false);
});

it('hides the sidebar if window is resized', () => {
const sidebar = createSidebar();
sidebar.show();
assert.isNotEmpty(sidebar.frame.style.marginLeft);

window.dispatchEvent(new Event('resize'));
assert.isEmpty(sidebar.frame.style.marginLeft);
});

describe('register/unregister events', () => {
it('triggers registered event listener', () => {
const sidebar = createSidebar();
const listener = sinon.stub();
sidebar._registerEvent(window, 'resize', listener);

window.dispatchEvent(new Event('resize'));
assert.calledOnce(listener);
});

it('unregisters event listeners', () => {
const sidebar = createSidebar();
const listener = sinon.stub();
sidebar._registerEvent(window, 'resize', listener);
sidebar.destroy();

window.dispatchEvent(new Event('resize'));
assert.notCalled(listener);
});
});

describe('layout change notifier', () => {
let layoutChangeHandlerSpy;

Expand Down