From cdea037b79ad4ae9f4510463d960788ea2971b3d Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 7 Nov 2019 10:20:24 -0800 Subject: [PATCH 1/3] refactor(controls): Adjusting the size of the grouped controls --- src/lib/Controls.js | 23 +++++++++++++++++++++-- src/lib/Controls.scss | 26 ++++++++++++++++++++++++-- src/lib/PageControls.js | 26 ++++++++++++++++++++++---- src/lib/ZoomControls.js | 26 ++++++++++++++++++++++---- src/lib/__tests__/Controls-test.js | 14 ++++++++++++++ src/lib/constants.js | 1 + 6 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/lib/Controls.js b/src/lib/Controls.js index f499011a5..2e3758d79 100644 --- a/src/lib/Controls.js +++ b/src/lib/Controls.js @@ -191,9 +191,12 @@ class Controls { * @param {string} [classList] - optional class list * @param {string} [content] - Optional content HTML * @param {string} [tag] - Optional html tag, defaults to 'button' + * @param {HTMLElement} [parent] - Optional parent tag, defaults to the controls element * @return {HTMLElement} The created HTMLElement inserted into the control */ - add(text, handler, classList = '', content = '', tag = 'button') { + add(text, handler, classList = '', content = '', tag = 'button', parent = this.controlsEl) { + const parentElement = this.controlsEl.contains(parent) ? parent : this.controlsEl; + const cell = document.createElement('div'); cell.className = 'bp-controls-cell'; @@ -214,7 +217,7 @@ class Controls { } cell.appendChild(element); - this.controlsEl.appendChild(cell); + parentElement.appendChild(cell); if (handler) { // Maintain a reference for cleanup @@ -227,6 +230,22 @@ class Controls { return element; } + /** + * Add div for a group of controls + * + * @public + * @param {string} classNames - optional class names + * @return {HTMLElement} The created HTMLElement for a group of controls + */ + addGroup(classNames = '') { + const group = document.createElement('div'); + group.className = `bp-controls-group ${classNames}`; + + this.controlsEl.appendChild(group); + + return group; + } + /** * Enables the controls. * diff --git a/src/lib/Controls.scss b/src/lib/Controls.scss index 81835a76f..b5ea4a18e 100644 --- a/src/lib/Controls.scss +++ b/src/lib/Controls.scss @@ -10,8 +10,7 @@ .bp-controls { position: relative; left: -50%; - display: table; - table-layout: fixed; + display: flex; background: fade-out($twos, .05); border-radius: 3px; opacity: 0; @@ -81,6 +80,12 @@ visibility: visible; } } + + .bp-zoom-btn { + display: flex; + align-items: center; + justify-content: center; + } } .box-show-preview-controls .bp-controls { @@ -155,6 +160,23 @@ } .bp-zoom-current-scale { + min-width: 48px; color: $white; font-size: 14px; + text-align: center; +} + +.bp-controls-group { + display: flex; + align-items: center; + margin-right: 4px; + margin-left: 4px; + + .bp-controls-group-btn { + width: 32px; + } + + & + .bp-controls-cell { + margin-left: 4px; + } } diff --git a/src/lib/PageControls.js b/src/lib/PageControls.js index 84f0686aa..e97794a14 100644 --- a/src/lib/PageControls.js +++ b/src/lib/PageControls.js @@ -1,7 +1,7 @@ import EventEmitter from 'events'; import fullscreen from './Fullscreen'; import Browser from './Browser'; -import { BROWSERS } from './constants'; +import { BROWSERS, CLASS_BOX_CONTROLS_GROUP_BUTTON } from './constants'; import { decodeKeydown } from './util'; import { ICON_DROP_DOWN, ICON_DROP_UP } from './icons/icons'; @@ -72,14 +72,32 @@ class PageControls extends EventEmitter { * @return {void} */ add(currentPageNumber, pagesCount) { + const groupElement = this.controls.addGroup(); + // const groupElement = undefined; this.controls.add( __('previous_page'), this.setPreviousPage, - `bp-previous-page-icon ${PREV_PAGE}`, + `${CLASS_BOX_CONTROLS_GROUP_BUTTON} bp-previous-page-icon ${PREV_PAGE}`, ICON_DROP_UP, + undefined, + groupElement, + ); + this.controls.add( + __('enter_page_num'), + this.showPageNumInput, + PAGE_NUM, + pageNumTemplate, + undefined, + groupElement, + ); + this.controls.add( + __('next_page'), + this.setNextPage, + `${CLASS_BOX_CONTROLS_GROUP_BUTTON} bp-next-page-icon ${NEXT_PAGE}`, + ICON_DROP_DOWN, + undefined, + groupElement, ); - 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); const pageNumEl = this.controlsEl.querySelector(`.${PAGE_NUM}`); this.totalPagesEl = pageNumEl.querySelector(`.${CONTROLS_TOTAL_PAGES}`); diff --git a/src/lib/ZoomControls.js b/src/lib/ZoomControls.js index eb85505d2..4efc3682f 100644 --- a/src/lib/ZoomControls.js +++ b/src/lib/ZoomControls.js @@ -2,10 +2,12 @@ import isFinite from 'lodash/isFinite'; import noop from 'lodash/noop'; import { ICON_ZOOM_IN, ICON_ZOOM_OUT } from './icons/icons'; import Controls from './Controls'; +import { CLASS_BOX_CONTROLS_GROUP_BUTTON } from './constants'; const CLASS_ZOOM_CURRENT_SCALE = 'bp-zoom-current-scale'; const CLASS_ZOOM_IN_BUTTON = 'bp-zoom-in-btn'; const CLASS_ZOOM_OUT_BUTTON = 'bp-zoom-out-btn'; +const CLASS_ZOOM_BUTTON = 'bp-zoom-btn'; class ZoomControls { /** @property {Controls} - Controls object */ @@ -67,15 +69,31 @@ class ZoomControls { this.maxZoom = Math.round(this.validateZoom(maxZoom, Number.POSITIVE_INFINITY) * 100); this.minZoom = Math.round(Math.max(this.validateZoom(minZoom, 0), 0) * 100); - this.controls.add(__('zoom_out'), onZoomOut, `${CLASS_ZOOM_OUT_BUTTON} ${zoomOutClassName}`, ICON_ZOOM_OUT); + const groupElement = this.controls.addGroup(); this.controls.add( - __('zoom_current_scale'), + __('zoom_out'), + onZoomOut, + `${CLASS_BOX_CONTROLS_GROUP_BUTTON} ${CLASS_ZOOM_BUTTON} ${CLASS_ZOOM_OUT_BUTTON} ${zoomOutClassName}`, + ICON_ZOOM_OUT, undefined, + groupElement, + ); + this.controls.add( + __('zoom_current_scale'), undefined, - `100%`, + CLASS_ZOOM_CURRENT_SCALE, + '100%', 'div', + groupElement, + ); + this.controls.add( + __('zoom_in'), + onZoomIn, + `${CLASS_BOX_CONTROLS_GROUP_BUTTON} ${CLASS_ZOOM_BUTTON} ${CLASS_ZOOM_IN_BUTTON} ${zoomInClassName}`, + ICON_ZOOM_IN, + undefined, + groupElement, ); - this.controls.add(__('zoom_in'), onZoomIn, `${CLASS_ZOOM_IN_BUTTON} ${zoomInClassName}`, ICON_ZOOM_IN); this.currentScaleElement = this.controlsElement.querySelector(`.${CLASS_ZOOM_CURRENT_SCALE}`); this.setCurrentScale(currentScale); diff --git a/src/lib/__tests__/Controls-test.js b/src/lib/__tests__/Controls-test.js index 1695e2c10..82cfabf6a 100644 --- a/src/lib/__tests__/Controls-test.js +++ b/src/lib/__tests__/Controls-test.js @@ -284,6 +284,20 @@ describe('lib/Controls', () => { expect(span.parentNode.parentNode).to.equal(controls.controlsEl); expect(controls.buttonRefs.push).not.to.be.called; }); + + it('should append the controls to the provided element', () => { + const div = controls.addGroup('test-group'); + const btn = controls.add('test button', sandbox.stub(), 'test1', 'test content', undefined, div); + expect(btn.parentNode.parentNode).to.equal(div); + }); + }); + + describe('addGroup()', () => { + it('should create a controls group within the controls element', () => { + const div = controls.addGroup('test-group'); + expect(div.parentNode).to.equal(controls.controlsEl); + expect(div.classList.contains('test-group')); + }); }); describe('enable()', () => { diff --git a/src/lib/constants.js b/src/lib/constants.js index bfe39e786..28b412572 100644 --- a/src/lib/constants.js +++ b/src/lib/constants.js @@ -2,6 +2,7 @@ export const CLASS_ACTIVE = 'bp-is-active'; export const CLASS_NAVIGATION_VISIBILITY = 'bp-is-navigation-visible'; export const CLASS_HIDDEN = 'bp-is-hidden'; export const CLASS_PREVIEW_LOADED = 'bp-loaded'; +export const CLASS_BOX_CONTROLS_GROUP_BUTTON = 'bp-controls-group-btn'; export const CLASS_BOX_PREVIEW = 'bp'; export const CLASS_BOX_PREVIEW_BUTTON = 'bp-btn'; export const CLASS_BOX_PREVIEW_CONTAINER = 'bp-container'; From faccc17fc3cac3be21914fab4600440b307c16e5 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 7 Nov 2019 10:47:10 -0800 Subject: [PATCH 2/3] chore: fix unit tests --- src/lib/__tests__/ZoomControls-test.js | 32 +++++++++++++++++++++----- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/src/lib/__tests__/ZoomControls-test.js b/src/lib/__tests__/ZoomControls-test.js index 53411002c..163bfd2fe 100644 --- a/src/lib/__tests__/ZoomControls-test.js +++ b/src/lib/__tests__/ZoomControls-test.js @@ -48,15 +48,30 @@ describe('lib/ZoomControls', () => { it('should add the controls', () => { zoomControls.init(0.5, { onZoomIn: stubs.onZoomIn, onZoomOut: stubs.onZoomOut }); - expect(stubs.add).to.be.calledWith(__('zoom_out'), stubs.onZoomOut, 'bp-zoom-out-btn ', ICON_ZOOM_OUT); expect(stubs.add).to.be.calledWith( - __('zoom_current_scale'), + __('zoom_out'), + stubs.onZoomOut, + 'bp-controls-group-btn bp-zoom-btn bp-zoom-out-btn ', + ICON_ZOOM_OUT, undefined, + sinon.match.any, + ); + expect(stubs.add).to.be.calledWith( + __('zoom_current_scale'), undefined, + 'bp-zoom-current-scale', sinon.match.string, 'div', + sinon.match.any, + ); + expect(stubs.add).to.be.calledWith( + __('zoom_in'), + stubs.onZoomIn, + 'bp-controls-group-btn bp-zoom-btn bp-zoom-in-btn ', + ICON_ZOOM_IN, + undefined, + sinon.match.any, ); - expect(stubs.add).to.be.calledWith(__('zoom_in'), stubs.onZoomIn, 'bp-zoom-in-btn ', ICON_ZOOM_IN); expect(zoomControls.currentScaleElement).not.to.be.undefined; expect(stubs.setCurrentScale).to.be.calledWith(0.5); expect(zoomControls.maxZoom).to.be.equal(Number.POSITIVE_INFINITY); @@ -102,21 +117,26 @@ describe('lib/ZoomControls', () => { expect(stubs.add).to.be.calledWith( __('zoom_out'), stubs.onZoomOut, - 'bp-zoom-out-btn zoom-out-classname', + 'bp-controls-group-btn bp-zoom-btn bp-zoom-out-btn zoom-out-classname', ICON_ZOOM_OUT, + undefined, + sinon.match.any, ); expect(stubs.add).to.be.calledWith( __('zoom_current_scale'), undefined, - undefined, + 'bp-zoom-current-scale', sinon.match.string, 'div', + sinon.match.any, ); expect(stubs.add).to.be.calledWith( __('zoom_in'), stubs.onZoomIn, - 'bp-zoom-in-btn zoom-in-classname', + 'bp-controls-group-btn bp-zoom-btn bp-zoom-in-btn zoom-in-classname', ICON_ZOOM_IN, + undefined, + sinon.match.any, ); }); }); From fcd2fc9575cf9e5c924d0aa08c63192be38219f1 Mon Sep 17 00:00:00 2001 From: Conrad Chan Date: Thu, 7 Nov 2019 12:47:45 -0800 Subject: [PATCH 3/3] chore: PR comments and make svgs not focusable --- src/lib/Controls.js | 2 ++ src/lib/PageControls.js | 3 ++- src/lib/ZoomControls.js | 3 +-- src/lib/constants.js | 1 - src/lib/icons/search.svg | 2 +- src/lib/icons/thumbnails-toggle-icon.svg | 2 +- src/lib/icons/zoom_in.svg | 2 +- src/lib/icons/zoom_out.svg | 2 +- 8 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/lib/Controls.js b/src/lib/Controls.js index 2e3758d79..58d9b22d0 100644 --- a/src/lib/Controls.js +++ b/src/lib/Controls.js @@ -8,6 +8,8 @@ const CONTROLS_PAGE_NUM_INPUT_CLASS = 'bp-page-num-input'; const CONTROLS_PAGE_NUM_WRAPPER_CLASS = 'bp-page-num-wrapper'; const CONTROLS_AUTO_HIDE_TIMEOUT_IN_MILLIS = 2000; +export const CLASS_BOX_CONTROLS_GROUP_BUTTON = 'bp-controls-group-btn'; + class Controls { /** @property {HTMLElement} - Controls container element */ containerEl; diff --git a/src/lib/PageControls.js b/src/lib/PageControls.js index e97794a14..b2a9a7972 100644 --- a/src/lib/PageControls.js +++ b/src/lib/PageControls.js @@ -1,9 +1,10 @@ import EventEmitter from 'events'; import fullscreen from './Fullscreen'; import Browser from './Browser'; -import { BROWSERS, CLASS_BOX_CONTROLS_GROUP_BUTTON } from './constants'; +import { BROWSERS } from './constants'; import { decodeKeydown } from './util'; import { ICON_DROP_DOWN, ICON_DROP_UP } from './icons/icons'; +import { CLASS_BOX_CONTROLS_GROUP_BUTTON } from './Controls'; const SHOW_PAGE_NUM_INPUT_CLASS = 'show-page-number-input'; const CONTROLS_PAGE_NUM_WRAPPER_CLASS = 'bp-page-num-wrapper'; diff --git a/src/lib/ZoomControls.js b/src/lib/ZoomControls.js index 4efc3682f..767db517a 100644 --- a/src/lib/ZoomControls.js +++ b/src/lib/ZoomControls.js @@ -1,8 +1,7 @@ import isFinite from 'lodash/isFinite'; import noop from 'lodash/noop'; import { ICON_ZOOM_IN, ICON_ZOOM_OUT } from './icons/icons'; -import Controls from './Controls'; -import { CLASS_BOX_CONTROLS_GROUP_BUTTON } from './constants'; +import Controls, { CLASS_BOX_CONTROLS_GROUP_BUTTON } from './Controls'; const CLASS_ZOOM_CURRENT_SCALE = 'bp-zoom-current-scale'; const CLASS_ZOOM_IN_BUTTON = 'bp-zoom-in-btn'; diff --git a/src/lib/constants.js b/src/lib/constants.js index 28b412572..bfe39e786 100644 --- a/src/lib/constants.js +++ b/src/lib/constants.js @@ -2,7 +2,6 @@ export const CLASS_ACTIVE = 'bp-is-active'; export const CLASS_NAVIGATION_VISIBILITY = 'bp-is-navigation-visible'; export const CLASS_HIDDEN = 'bp-is-hidden'; export const CLASS_PREVIEW_LOADED = 'bp-loaded'; -export const CLASS_BOX_CONTROLS_GROUP_BUTTON = 'bp-controls-group-btn'; export const CLASS_BOX_PREVIEW = 'bp'; export const CLASS_BOX_PREVIEW_BUTTON = 'bp-btn'; export const CLASS_BOX_PREVIEW_CONTAINER = 'bp-container'; diff --git a/src/lib/icons/search.svg b/src/lib/icons/search.svg index 6b4ce0e09..e497efe42 100644 --- a/src/lib/icons/search.svg +++ b/src/lib/icons/search.svg @@ -1,4 +1,4 @@ - + diff --git a/src/lib/icons/thumbnails-toggle-icon.svg b/src/lib/icons/thumbnails-toggle-icon.svg index f7644db48..e8292bc9a 100644 --- a/src/lib/icons/thumbnails-toggle-icon.svg +++ b/src/lib/icons/thumbnails-toggle-icon.svg @@ -1 +1 @@ - + diff --git a/src/lib/icons/zoom_in.svg b/src/lib/icons/zoom_in.svg index 8a4f5f460..ab345c490 100644 --- a/src/lib/icons/zoom_in.svg +++ b/src/lib/icons/zoom_in.svg @@ -1,4 +1,4 @@ - + diff --git a/src/lib/icons/zoom_out.svg b/src/lib/icons/zoom_out.svg index 4f04ceccd..908c12379 100644 --- a/src/lib/icons/zoom_out.svg +++ b/src/lib/icons/zoom_out.svg @@ -1,4 +1,4 @@ - +