refactor(controls): Adjusting the size of the grouped controls (#1096)

* refactor(controls): Adjusting the size of the grouped controls * chore: fix unit tests * chore: PR comments and make svgs not focusable
box · Nov 7, 2019 · b04e7e6 · b04e7e6
1 parent fcae150
commit b04e7e6
Show file tree

Hide file tree

Showing 10 changed files with 135 additions and 22 deletions.
diff --git 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;
@@ -191,9 +193,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 +219,7 @@ class Controls {
         }
 
         cell.appendChild(element);
-        this.controlsEl.appendChild(cell);
+        parentElement.appendChild(cell);
 
         if (handler) {
             // Maintain a reference for cleanup
@@ -227,6 +232,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
@@ -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
@@ -4,6 +4,7 @@ import Browser from './Browser';
 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';
@@ -72,14 +73,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
@@ -1,11 +1,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 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';
 const CLASS_ZOOM_OUT_BUTTON = 'bp-zoom-out-btn';
+const CLASS_ZOOM_BUTTON = 'bp-zoom-btn';
 
 class ZoomControls {
     /** @property {Controls} - Controls object */
@@ -67,15 +68,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,
-            `<span class="${CLASS_ZOOM_CURRENT_SCALE}" data-testid="current-zoom">100%</span>`,
+            CLASS_ZOOM_CURRENT_SCALE,
+            '<span data-testid="current-zoom">100%</span>',
             '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
@@ -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/__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,
             );
         });
     });

diff --git a/src/lib/icons/search.svg b/src/lib/icons/search.svg
diff --git a/src/lib/icons/thumbnails-toggle-icon.svg b/src/lib/icons/thumbnails-toggle-icon.svg
diff --git a/src/lib/icons/zoom_in.svg b/src/lib/icons/zoom_in.svg
diff --git a/src/lib/icons/zoom_out.svg b/src/lib/icons/zoom_out.svg