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

Jump to bottom

New: Allow scrolling for multi-page image files #308

Merged
merged 12 commits into from Aug 16, 2017
Merged

New: Allow scrolling for multi-page image files #308

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
d1434f1
New: Allow scrolling for multi-page image files
pramodsum Aug 15, 2017
a9ebfc6
Chore: unit tests
pramodsum Aug 15, 2017
8c52781
New: Adding page number info/controls to the MultiImageViewer
pramodsum Aug 15, 2017
f1580d8
New: Updating multi-image page numbers on scroll
pramodsum Aug 15, 2017
8da02a4
Chore: Moving strings into constants
pramodsum Aug 15, 2017
9e6a9ec
Fix: Only check next page if one exists
pramodsum Aug 15, 2017
d266bea
Chore: PR cleanup
pramodsum Aug 15, 2017
e13756a
Chore: unit tests
pramodsum Aug 15, 2017
8cdc415
Chore: The rest of the unit tests
pramodsum Aug 16, 2017
10876d7
Merge branch 'master' into annotations
pramodsum Aug 16, 2017
c356a5f
Chore: last unit tests fix
pramodsum Aug 16, 2017
8a0467d
Chore: Actually using the correct previousPage() method
pramodsum Aug 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 76 additions & 0 deletions src/lib/Controls.js
View file
Open in desktop
@@ -1,6 +1,7 @@
import throttle from 'lodash.throttle';
import Browser from './Browser';
import { CLASS_HIDDEN } from './constants';
import fullscreen from './Fullscreen';

const SHOW_PREVIEW_CONTROLS_CLASS = 'box-show-preview-controls';
const CONTROLS_BUTTON_CLASS = 'bp-controls-btn';
Expand All @@ -24,6 +25,18 @@ class Controls {
/** @property {boolean} - Whether browser supports touch */
hasTouch = Browser.hasTouch();

/** @property {HTMLElement} - Page num input element */
pageNumInputEl;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removing


/** @property {String} - HTML template for page num element */
pageNumTemplate = `
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of making this a property of Controls.js (since it is not required for base controls, and is used as a constant) could you make this an exported constant

<div class='bp-page-num-wrapper'>
<span class='bp-current-page'>1</span>
<input type='number' pattern='[0-9]*' min='1' value='' size='3' class='bp-page-num-input' />
<span class='bp-page-num-divider'>&nbsp;/&nbsp;</span>
<span class='bp-total-pages'>1</span>
</div>`.replace(/>\s*</g, '><');

/**
* [constructor]
*
Expand Down Expand Up @@ -239,6 +252,69 @@ class Controls {
isPageNumFocused() {
return document.activeElement.classList.contains(CONTROLS_PAGE_NUM_INPUT_CLASS);
}

/**
* Initializes page number selector.
*
* @private
* @param {number} pagesCount - Total number of page
* @return {void}
*/
initPageNumEl(pagesCount) {
const pageNumEl = this.controlsEl.querySelector('.bp-page-num');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you consolidate re-used selectors into constants?

IE) .bp-page-num


// Update total page number
const totalPageEl = pageNumEl.querySelector('.bp-total-pages');
totalPageEl.textContent = pagesCount;

// Keep reference to page number input and current page elements
this.pageNumInputEl = pageNumEl.querySelector('.bp-page-num-input');
this.pageNumInputEl.setAttribute('max', pagesCount);

this.currentPageEl = pageNumEl.querySelector('.bp-current-page');
}

/**
* Disables or enables previous/next pagination buttons depending on
* current page number.
*
* @return {void}
*/
checkPaginationButtons(currentPageNum, pagesCount) {
const pageNumButtonEl = this.containerEl.querySelector('.bp-page-num');
const previousPageButtonEl = this.containerEl.querySelector('.bp-previous-page');
const nextPageButtonEl = this.containerEl.querySelector('.bp-next-page');

// Safari disables keyboard input in fullscreen before Safari 10.1
const isSafariFullscreen = Browser.getName() === 'Safari' && fullscreen.isFullscreen(this.containerEl);

// Disable page number selector if there is only one page or less
if (pageNumButtonEl) {
if (pagesCount <= 1 || isSafariFullscreen) {
pageNumButtonEl.disabled = true;
} else {
pageNumButtonEl.disabled = false;
}
}

// Disable previous page if on first page, otherwise enable
if (previousPageButtonEl) {
if (currentPageNum === 1) {
previousPageButtonEl.disabled = true;
} else {
previousPageButtonEl.disabled = false;
}
}

// Disable next page if on last page, otherwise enable
if (nextPageButtonEl) {
if (currentPageNum === pagesCount) {
nextPageButtonEl.disabled = true;
} else {
nextPageButtonEl.disabled = false;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you imagine if we had React?

}

export default Controls;
65 changes: 65 additions & 0 deletions src/lib/Controls.scss
View file
Open in desktop
Expand Up @@ -16,6 +16,71 @@
position: relative;
table-layout: fixed;
transition: opacity .5s;

// Page num input CSS
.bp-page-num {
min-width: 48px;
width: auto; // Let page num expand as needed

span {
display: inline;
font-size: 14px;
}
}

.bp-page-num-wrapper {
background-color: #444;
border-radius: 3px;
margin: 5px;
padding: 7px 5px;
}

/* stylelint-disable property-no-vendor-prefix */
// Removes the spinner for number type inputs in Webkit browsers
input::-webkit-outer-spin-button,
input::-webkit-inner-spin-button {
-webkit-appearance: none;
}

// Removes the spinner for number type inputs in Firefox
input[type=number] {
-moz-appearance: textfield;
}

/* stylelint-enable property-no-vendor-prefix */

.bp-page-num-input {
font-size: 14px;
margin: 0 auto;
position: absolute;
text-align: center;
visibility: hidden;
width: 44px; // hard-coded to solve layout issues
}

&.show-page-number-input {
.bp-page-num-wrapper {
background-color: transparent;
border: none;
padding: 0;
}

.bp-page-num {
opacity: 1;
}

.bp-current-page,
.bp-page-num-divider,
.bp-total-pages {
display: none;
}

.bp-page-num-input {
display: inline-block;
position: static;
visibility: visible;
}
}
}

.box-show-preview-controls .bp-controls {
Expand Down
92 changes: 14 additions & 78 deletions src/lib/viewers/doc/DocBaseViewer.js
View file
Open in desktop
Expand Up @@ -347,50 +347,6 @@ class DocBaseViewer extends BaseViewer {
this.cache.set(CURRENT_PAGE_MAP_KEY, currentPageMap, true /* useLocalStorage */);
}

/**
* Disables or enables previous/next pagination buttons depending on
* current page number.
*
* @return {void}
*/
checkPaginationButtons() {
const pagesCount = this.pdfViewer.pagesCount;
const currentPageNum = this.pdfViewer.currentPageNumber;
const pageNumButtonEl = this.containerEl.querySelector('.bp-doc-page-num');
const previousPageButtonEl = this.containerEl.querySelector('.bp-previous-page');
const nextPageButtonEl = this.containerEl.querySelector('.bp-next-page');

// Safari disables keyboard input in fullscreen before Safari 10.1
const isSafariFullscreen = Browser.getName() === 'Safari' && fullscreen.isFullscreen(this.containerEl);

// Disable page number selector if there is only one page or less
if (pageNumButtonEl) {
if (pagesCount <= 1 || isSafariFullscreen) {
pageNumButtonEl.disabled = true;
} else {
pageNumButtonEl.disabled = false;
}
}

// Disable previous page if on first page, otherwise enable
if (previousPageButtonEl) {
if (currentPageNum === 1) {
previousPageButtonEl.disabled = true;
} else {
previousPageButtonEl.disabled = false;
}
}

// Disable next page if on last page, otherwise enable
if (nextPageButtonEl) {
if (currentPageNum === this.pdfViewer.pagesCount) {
nextPageButtonEl.disabled = true;
} else {
nextPageButtonEl.disabled = false;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

+100000 for removing UI stuff from the Viewer :)


/**
* Zoom into document.
*
Expand Down Expand Up @@ -663,26 +619,6 @@ class DocBaseViewer extends BaseViewer {
});
}

/**
* Initializes page number selector.
*
* @private
* @return {void}
*/
initPageNumEl() {
const pageNumEl = this.controls.controlsEl.querySelector('.bp-doc-page-num');

// Update total page number
const totalPageEl = pageNumEl.querySelector('.bp-doc-total-pages');
totalPageEl.textContent = this.pdfViewer.pagesCount;

// Keep reference to page number input and current page elements
this.pageNumInputEl = pageNumEl.querySelector('.bp-doc-page-num-input');
this.pageNumInputEl.setAttribute('max', this.pdfViewer.pagesCount);

this.currentPageEl = pageNumEl.querySelector('.bp-doc-current-page');
}

/**
* Fetches PDF and converts to blob for printing.
*
Expand Down Expand Up @@ -761,7 +697,7 @@ class DocBaseViewer extends BaseViewer {
loadUI() {
this.controls = new Controls(this.containerEl);
this.bindControlListeners();
this.initPageNumEl();
this.controls.initPageNumEl(this.pdfViewer.pagesCount);
}

/**
Expand All @@ -774,13 +710,13 @@ class DocBaseViewer extends BaseViewer {
// show the input box with the current page number selected within it
this.controls.controlsEl.classList.add(SHOW_PAGE_NUM_INPUT_CLASS);

this.pageNumInputEl.value = this.currentPageEl.textContent;
this.pageNumInputEl.focus();
this.pageNumInputEl.select();
this.controls.pageNumInputEl.value = this.controls.currentPageEl.textContent;
this.controls.pageNumInputEl.focus();
this.controls.pageNumInputEl.select();

// finish input when input is blurred or enter key is pressed
this.pageNumInputEl.addEventListener('blur', this.pageNumInputBlurHandler);
this.pageNumInputEl.addEventListener('keydown', this.pageNumInputKeydownHandler);
this.controls.pageNumInputEl.addEventListener('blur', this.pageNumInputBlurHandler);
this.controls.pageNumInputEl.addEventListener('keydown', this.pageNumInputKeydownHandler);
}

/**
Expand All @@ -791,8 +727,8 @@ class DocBaseViewer extends BaseViewer {
*/
hidePageNumInput() {
this.controls.controlsEl.classList.remove(SHOW_PAGE_NUM_INPUT_CLASS);
this.pageNumInputEl.removeEventListener('blur', this.pageNumInputBlurHandler);
this.pageNumInputEl.removeEventListener('keydown', this.pageNumInputKeydownHandler);
this.controls.pageNumInputEl.removeEventListener('blur', this.pageNumInputBlurHandler);
this.controls.pageNumInputEl.removeEventListener('keydown', this.pageNumInputKeydownHandler);
}

/**
Expand All @@ -813,15 +749,15 @@ class DocBaseViewer extends BaseViewer {
truePageNum = 1;
}

if (this.pageNumInputEl) {
this.pageNumInputEl.value = truePageNum;
if (this.controls.pageNumInputEl) {
this.controls.pageNumInputEl.value = truePageNum;
}

if (this.currentPageEl) {
this.currentPageEl.textContent = truePageNum;
if (this.controls.currentPageEl) {
this.controls.currentPageEl.textContent = truePageNum;
}

this.checkPaginationButtons();
this.controls.checkPaginationButtons(this.pdfViewer.currentPageNumber, this.pdfViewer.pagesCount);
}

//--------------------------------------------------------------------------
Expand Down Expand Up @@ -969,7 +905,7 @@ class DocBaseViewer extends BaseViewer {
this.pdfViewer.currentScaleValue = 'auto';

this.loadUI();
this.checkPaginationButtons();
this.controls.checkPaginationButtons(this.pdfViewer.currentPageNumber, this.pdfViewer.pagesCount);

// Set current page to previously opened page or first page
this.setPage(this.getCachedPage());
Expand Down
5 changes: 1 addition & 4 deletions src/lib/viewers/doc/DocumentViewer.js
View file
Open in desktop
@@ -1,5 +1,4 @@
import autobind from 'autobind-decorator';
import pageNumTemplate from './pageNumButtonContent.html';
import DocBaseViewer from './DocBaseViewer';
import DocPreloader from './DocPreloader';
import fullscreen from '../../Fullscreen';
Expand Down Expand Up @@ -109,9 +108,7 @@ class DocumentViewer extends DocBaseViewer {
'bp-doc-previous-page-icon bp-previous-page',
ICON_DROP_UP
);

const buttonContent = pageNumTemplate.replace(/>\s*</g, '><'); // removing new lines
this.controls.add(__('enter_page_num'), this.showPageNumInput, 'bp-doc-page-num', buttonContent);
this.controls.add(__('enter_page_num'), this.showPageNumInput, 'bp-page-num', this.controls.pageNumTemplate);
this.controls.add(__('next_page'), this.nextPage, 'bp-doc-next-page-icon bp-next-page', ICON_DROP_DOWN);

this.controls.add(
Expand Down
5 changes: 1 addition & 4 deletions src/lib/viewers/doc/PresentationViewer.js
View file
Open in desktop
@@ -1,6 +1,5 @@
import autobind from 'autobind-decorator';
import throttle from 'lodash.throttle';
import pageNumTemplate from './pageNumButtonContent.html';
import DocBaseViewer from './DocBaseViewer';
import PresentationPreloader from './PresentationPreloader';
import { CLASS_INVISIBLE } from '../../constants';
Expand Down Expand Up @@ -199,9 +198,7 @@ class PresentationViewer extends DocBaseViewer {
'bp-presentation-previous-page-icon bp-previous-page',
ICON_DROP_UP
);

const buttonContent = pageNumTemplate.replace(/>\s*</g, '><'); // removing new lines
this.controls.add(__('enter_page_num'), this.showPageNumInput, 'bp-doc-page-num', buttonContent);
this.controls.add(__('enter_page_num'), this.showPageNumInput, 'bp-page-num', this.controls.pageNumTemplate);
Copy link
Contributor

Choose a reason for hiding this comment

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

constant for bp-page-num, since it's used in multiple places?


this.controls.add(
__('next_page'),
Expand Down