From 46e1d5daa4eb53e68e05ae10de512ca38f9e7c6c Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 8 Jul 2018 10:55:56 +0200 Subject: [PATCH 1/3] Simplify resetting of the `SecondaryToolbar` Scroll/Spread mode buttons, and add a missing comment in `PDFCursorTools` The names 'resetscrollmode'/'resetspreadmode' were probably *not* great choices, given that the only thing being reset are toolbar buttons and not the actual Scroll/Spread modes. Furthermore, there's really no need for two separate events here. The patch also adds a comment that ought to have been included in PR 9040, to prevent future refactoring/removing of what may appear to be an unnecessary `Promise.resolve` call. --- web/pdf_cursor_tools.js | 2 ++ web/secondary_toolbar.js | 7 +++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/web/pdf_cursor_tools.js b/web/pdf_cursor_tools.js index 8d230a5981c1d..2529639448198 100644 --- a/web/pdf_cursor_tools.js +++ b/web/pdf_cursor_tools.js @@ -47,6 +47,8 @@ class PDFCursorTools { this._addEventListeners(); + // Defer the initial `switchTool` call, to give other viewer components + // time to initialize *and* register 'cursortoolchanged' event listeners. Promise.resolve().then(() => { this.switchTool(cursorToolOnLoad); }); diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js index ac46becdeb9dd..2b59a93d02a25 100644 --- a/web/secondary_toolbar.js +++ b/web/secondary_toolbar.js @@ -142,8 +142,7 @@ class SecondaryToolbar { this._updateUIState(); // Reset the Scroll/Spread buttons too, since they're document specific. - this.eventBus.dispatch('resetscrollmode', { source: this, }); - this.eventBus.dispatch('resetspreadmode', { source: this, }); + this.eventBus.dispatch('secondarytoolbarreset', { source: this, }); } _updateUIState() { @@ -212,7 +211,7 @@ class SecondaryToolbar { } this.eventBus.on('scrollmodechanged', scrollModeChanged); - this.eventBus.on('resetscrollmode', (evt) => { + this.eventBus.on('secondarytoolbarreset', (evt) => { if (evt.source === this) { scrollModeChanged({ mode: ScrollMode.VERTICAL, }); } @@ -239,7 +238,7 @@ class SecondaryToolbar { } this.eventBus.on('spreadmodechanged', spreadModeChanged); - this.eventBus.on('resetspreadmode', (evt) => { + this.eventBus.on('secondarytoolbarreset', (evt) => { if (evt.source === this) { spreadModeChanged({ mode: SpreadMode.NONE, }); } From 36d6255866f1fd647b5d265e3d170129797bf30b Mon Sep 17 00:00:00 2001 From: Jonas Jenwald Date: Sun, 8 Jul 2018 10:56:01 +0200 Subject: [PATCH 2/3] Hide the Scroll/Spread mode buttons when the viewer is a `PDFSinglePageViewer` instance If the current viewer is a `PDFSinglePageViewer` instance the Scroll/Spread modes are no-ops, hence displaying buttons that do *nothing* when clicked will probably do very little besides confuse users. --- web/base_viewer.js | 6 ++++++ web/secondary_toolbar.js | 13 +++++++++++++ web/viewer.css | 5 +++++ web/viewer.html | 16 ++++++++-------- 4 files changed, 32 insertions(+), 8 deletions(-) diff --git a/web/base_viewer.js b/web/base_viewer.js index 4149dd3a21456..6a9bacc42a3a9 100644 --- a/web/base_viewer.js +++ b/web/base_viewer.js @@ -183,6 +183,12 @@ class BaseViewer { this.spreadMode = options.spreadMode; } } + + // Defer the dispatching of this event, to give other viewer components + // time to initialize *and* register 'baseviewerinit' event listeners. + Promise.resolve().then(() => { + this.eventBus.dispatch('baseviewerinit', { source: this, }); + }); } get pagesCount() { diff --git a/web/secondary_toolbar.js b/web/secondary_toolbar.js index 2b59a93d02a25..3eaabf9c17962 100644 --- a/web/secondary_toolbar.js +++ b/web/secondary_toolbar.js @@ -15,6 +15,7 @@ import { ScrollMode, SpreadMode } from './base_viewer'; import { CursorTool } from './pdf_cursor_tools'; +import { PDFSinglePageViewer } from './pdf_single_page_viewer'; import { SCROLLBAR_PADDING } from './ui_utils'; /** @@ -117,6 +118,18 @@ class SecondaryToolbar { // Bind the event listener for adjusting the 'max-height' of the toolbar. this.eventBus.on('resize', this._setMaxHeight.bind(this)); + + // Hide the Scroll/Spread mode buttons, when they're not applicable to the + // current `BaseViewer` instance (in particular `PDFSinglePageViewer`). + this.eventBus.on('baseviewerinit', (evt) => { + if (evt.source instanceof PDFSinglePageViewer) { + this.toolbarButtonContainer.classList.add('hiddenScrollModeButtons'); + this.toolbarButtonContainer.classList.add('hiddenSpreadModeButtons'); + } else { + this.toolbarButtonContainer.classList.remove('hiddenScrollModeButtons'); + this.toolbarButtonContainer.classList.remove('hiddenSpreadModeButtons'); + } + }); } /** diff --git a/web/viewer.css b/web/viewer.css index 2a87b025c938b..72230783dd3e3 100644 --- a/web/viewer.css +++ b/web/viewer.css @@ -394,6 +394,11 @@ html[dir='rtl'] .secondaryToolbar { margin-bottom: -4px; } +#secondaryToolbarButtonContainer.hiddenScrollModeButtons > .scrollModeButtons, +#secondaryToolbarButtonContainer.hiddenSpreadModeButtons > .spreadModeButtons { + display: none !important; +} + .doorHanger, .doorHangerRight { border: 1px solid hsla(0,0%,0%,.5); diff --git a/web/viewer.html b/web/viewer.html index ba0ac8e492eb6..5d64c1ed1b834 100644 --- a/web/viewer.html +++ b/web/viewer.html @@ -168,29 +168,29 @@
- - - -
+
- - - -
+