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

Chore: Refactor pageControls and scroll handling for multi page images #321

Merged
merged 4 commits into from
Aug 29, 2017
Merged

Chore: Refactor pageControls and scroll handling for multi page images #321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
70edfdb
Chore: Refactor pageControls and scroll handling for multi page images
Aug 17, 2017
051a96f
Chore: fix conflicts, tests, and small changes
Aug 19, 2017
f2ad9f8
Merge branch 'master' into refactor
Aug 28, 2017
120e0b6
Merge branch 'master' into refactor
Aug 29, 2017
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
1 change: 1 addition & 0 deletions src/lib/Controls.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@
margin: 0;
padding: 0;
touch-action: manipulation;
user-select: none;
vertical-align: middle;
}

Expand Down
170 changes: 115 additions & 55 deletions src/lib/PageControls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,51 +22,71 @@ const pageNumTemplate = `
</div>`.replace(/>\s*</g, '><');

class PageControls extends EventEmitter {
/** @property {Controls} - Controls object */
controls;

/** @property {HTMLElement} - Controls element */
controlsEl;

/** @property {HTMLElement} - File content element */
contentEl;

/** @property {HTMLElement} - Total pages element */
totalPagesEl;

/** @property {HTMLElement} - Current page element */
currentPageEl;

/** @property {HTMLElement} - Page number input element */
pageNumInputEl;
/**
* [constructor]
*
* @param {HTMLElement} controls - Viewer controls
* @param {Function} previousPage - Previous page handler
* @param {Function} nextPage - Next page handler
* @param {HTMLElement} contentEl - The content element of the file

* @return {Controls} Instance of controls
*/
constructor(controls, previousPage, nextPage) {
constructor(controls, contentEl) {
super();

this.controls = controls;
this.controlsEl = controls.controlsEl;
this.currentPageEl = controls.currentPageEl;
this.pageNumInputEl = controls.pageNumInputEl;
this.previousPage = previousPage;
this.nextPage = nextPage;

this.contentEl = contentEl;

this.pageNumInputBlurHandler = this.pageNumInputBlurHandler.bind(this);
this.pageNumInputKeydownHandler = this.pageNumInputKeydownHandler.bind(this);
this.setPreviousPage = this.setPreviousPage.bind(this);
this.showPageNumInput = this.showPageNumInput.bind(this);
this.setNextPage = this.setNextPage.bind(this);
}

/**
* Adds controls and initializes page number selector.
* Add the page controls
*
* @private
* @param {number} pagesCount - Total number of page
* @param {number} currentPageNumber - Current page number
* @param {number} pagesCount - Number of total pages
* @return {void}
*/
init(pagesCount) {
// Add controls
this.controls.add(__('previous_page'), this.previousPage, `bp-previous-page-icon ${PREV_PAGE}`, ICON_DROP_UP);
this.controls.add(__('enter_page_num'), this.showPageNumInput.bind(this), PAGE_NUM, pageNumTemplate);
this.controls.add(__('next_page'), this.nextPage, `bp-next-page-icon ${NEXT_PAGE}`, ICON_DROP_DOWN);
add(currentPageNumber, pagesCount) {
this.controls.add(
__('previous_page'),
this.setPreviousPage,
`bp-previous-page-icon ${PREV_PAGE}`,
ICON_DROP_UP
);
this.controls.add(__('enter_page_num'), this.showPageNumInput, PAGE_NUM, pageNumTemplate);
this.controls.add(__('next_page'), this.setNextPage, `bp-next-page-icon ${NEXT_PAGE}`, ICON_DROP_DOWN);

// Initialize page number selector
const pageNumEl = this.controlsEl.querySelector(`.${PAGE_NUM}`);
this.pagesCount = pagesCount;

// Update total page number
const totalPageEl = pageNumEl.querySelector(`.${CONTROLS_TOTAL_PAGES}`);
totalPageEl.textContent = pagesCount;

// Keep reference to page number input and current page elements
this.totalPagesEl = pageNumEl.querySelector(`.${CONTROLS_TOTAL_PAGES}`);
this.totalPagesEl.textContent = pagesCount;
this.currentPageEl = pageNumEl.querySelector(`.${CONTROLS_CURRENT_PAGE}`);
this.currentPageEl.textContent = currentPageNumber;
this.pageNumInputEl = pageNumEl.querySelector(`.${CONTROLS_PAGE_NUM_INPUT_CLASS}`);
this.pageNumInputEl.setAttribute('max', pagesCount);

this.currentPageEl = pageNumEl.querySelector(`.${CONTROLS_CURRENT_PAGE}`);
this.checkPaginationButtons();
}

/**
Expand All @@ -79,13 +99,13 @@ class PageControls extends EventEmitter {
// show the input box with the current page number selected within it
this.controlsEl.classList.add(SHOW_PAGE_NUM_INPUT_CLASS);

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

// finish input when input is blurred or enter key is pressed
this.pageNumInputEl.addEventListener('blur', this.pageNumInputBlurHandler.bind(this));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pramodsum bind creates a new function, which caused a new listener to get added each time this was called. It also wasn't being removed correctly as a result.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah good catch!

this.pageNumInputEl.addEventListener('keydown', this.pageNumInputKeydownHandler.bind(this));
this.pageNumInputEl.addEventListener('blur', this.pageNumInputBlurHandler);
this.pageNumInputEl.addEventListener('keydown', this.pageNumInputKeydownHandler);
}

/**
Expand All @@ -104,11 +124,9 @@ class PageControls extends EventEmitter {
* Disables or enables previous/next pagination buttons depending on
* current page number.
*
* @param {number} currentPageNum - Current page number
* @param {number} pagesCount - Total number of page
* @return {void}
*/
checkPaginationButtons(currentPageNum, pagesCount) {
checkPaginationButtons() {
const pageNumButtonEl = this.controlsEl.querySelector(`.${PAGE_NUM}`);
const previousPageButtonEl = this.controlsEl.querySelector(`.${PREV_PAGE}`);
const nextPageButtonEl = this.controlsEl.querySelector(`.${NEXT_PAGE}`);
Expand All @@ -118,7 +136,7 @@ class PageControls extends EventEmitter {

// Disable page number selector if there is only one page or less
if (pageNumButtonEl) {
if (pagesCount <= 1 || isSafariFullscreen) {
if (this.getTotalPages() <= 1 || isSafariFullscreen) {
pageNumButtonEl.disabled = true;
} else {
pageNumButtonEl.disabled = false;
Expand All @@ -127,7 +145,7 @@ class PageControls extends EventEmitter {

// Disable previous page if on first page, otherwise enable
if (previousPageButtonEl) {
if (currentPageNum === 1) {
if (this.getCurrentPageNumber() === 1) {
previousPageButtonEl.disabled = true;
} else {
previousPageButtonEl.disabled = false;
Expand All @@ -136,7 +154,7 @@ class PageControls extends EventEmitter {

// Disable next page if on last page, otherwise enable
if (nextPageButtonEl) {
if (currentPageNum === pagesCount) {
if (this.getCurrentPageNumber() === this.getTotalPages()) {
nextPageButtonEl.disabled = true;
} else {
nextPageButtonEl.disabled = false;
Expand All @@ -147,30 +165,71 @@ class PageControls extends EventEmitter {
/**
* Update page number in page control widget.
*
* @private
* @param {number} pageNum - Number of page to update to
* @public
* @param {number} pageNumber - Number of page to update to
* @return {void}
*/
updateCurrentPage(pageNum) {
let truePageNum = pageNum;

// refine the page number to fall within bounds
if (pageNum > this.pagesCount) {
truePageNum = this.pagesCount;
} else if (pageNum < 1) {
truePageNum = 1;
}

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

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

this.currentPageNumber = truePageNum;
this.checkPaginationButtons(this.currentPageNumber, this.pagesCount);
this.checkPaginationButtons();
}

/**
* Emits a message to the viewer to decrement the current page.
*
* @private
* @return {void}
*/
setPreviousPage() {
this.emit('pagechange', this.getCurrentPageNumber() - 1);
}

/**
* Emits a message to the viewer to increment the current page.
*
* @private
* @return {void}
*/
setNextPage() {
this.emit('pagechange', this.getCurrentPageNumber() + 1);
}

/**
* Gets the page number for the file.
*
* @private
* @return {number} Number of pages
*/
getCurrentPageNumber() {
return parseInt(this.currentPageEl.textContent, 10);
}

/**
* Sets the number of pages for the file.
*
* @private
* @param {number} pageNumber - Number to set
* @return {number} Number of pages
*/
setCurrentPageNumber(pageNumber) {
this.currentPageEl.textContent = pageNumber;
}

/**
* Gets the number of pages for the file.
*
* @private
* @return {number} Number of pages
*/
getTotalPages() {
return parseInt(this.totalPagesEl.textContent, 10);
}

/**
Expand All @@ -182,10 +241,10 @@ class PageControls extends EventEmitter {
*/
pageNumInputBlurHandler(event) {
const target = event.target;
const pageNum = parseInt(target.value, 10);
const pageNumber = parseInt(target.value, 10);

if (!isNaN(pageNum)) {
this.emit('setpage', pageNum);
if (!isNaN(pageNumber)) {
this.emit('pagechange', pageNumber);
}

this.hidePageNumInput();
Expand All @@ -204,12 +263,13 @@ class PageControls extends EventEmitter {
switch (key) {
case 'Enter':
case 'Tab':
this.contentEl.focus();
// The keycode of the 'next' key on Android Chrome is 9, which maps to 'Tab'.
// this.docEl.focus();
Copy link
Contributor

Choose a reason for hiding this comment

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

:( sorry!

// We normally trigger the blur handler by blurring the input
// field, but this doesn't work for IE in fullscreen. For IE,
// we blur the page behind the controls - this unfortunately
// is an IE-only solution that doesn't work with other browsers

if (Browser.getName() !== 'Explorer') {
event.target.blur();
}
Expand All @@ -220,7 +280,7 @@ class PageControls extends EventEmitter {

case 'Escape':
this.hidePageNumInput();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll verify this on Monday but I believe there's an IE 11 issue when hitting escape in fullscreen that also closes the preview. Pretty sure this is coming from the webapp side as they own that shortcut.

// this.docEl.focus();
this.contentEl.focus();

event.stopPropagation();
event.preventDefault();
Expand Down
Loading