Fix: Move notification to be child of preview content (#1019)

* Fix: Move notification to be child of preview content * Chore: use close icon in notification
box · Jun 19, 2019 · fde40a0 · fde40a0
1 parent 72eddb1
commit fde40a0
Show file tree

Hide file tree

Showing 10 changed files with 51 additions and 28 deletions.
diff --git a/src/i18n/en-US.properties b/src/i18n/en-US.properties
@@ -182,6 +182,8 @@ annotations_authorization_error=Your session has expired. Please refresh the pag
 # Notifications
 # Default text for notification button that dismisses notification
 notification_button_default_text=Okay
+# Default label for notification close button
+notification_button_default_label=Clear Notification
 # Notification message shown when user enters point annotation mode
 notification_annotation_point_mode=Click anywhere to add a comment to the document
 # Notification message shown when user enters drawing annotation mode

diff --git a/src/lib/Notification.js b/src/lib/Notification.js
@@ -1,4 +1,5 @@
 import { CLASS_HIDDEN, CLASS_BOX_PREVIEW_NOTIFICATION, CLASS_BOX_PREVIEW_NOTIFICATION_WRAPPER } from './constants';
+import { ICON_CLOSE } from './icons/icons';
 
 const HIDE_TIMEOUT_MS = 5000; // 5s
 
@@ -23,7 +24,7 @@ class Notification {
 
         this.notificationEl.innerHTML = `
             <span id="${uniqueLabel}"></span>
-            <button class="close-btn" type="button">${__('notification_button_default_text')}</button>
+            <button class="close-btn" type="button">${ICON_CLOSE}</button>
         `.trim();
 
         // Save references to message and button
@@ -51,8 +52,12 @@ class Notification {
 
         if (buttonText) {
             this.buttonEl.textContent = buttonText;
+            this.buttonEl.classList.remove('default-close-btn');
+            this.buttonEl.removeAttribute('aria-label');
         } else {
-            this.buttonEl.textContent = __('notification_button_default_text');
+            this.buttonEl.innerHTML = ICON_CLOSE;
+            this.buttonEl.classList.add('default-close-btn');
+            this.buttonEl.setAttribute('aria-label', __('notification_button_default_label'));
         }
 
         this.notificationEl.classList.remove(CLASS_HIDDEN);

diff --git a/src/lib/PreviewUI.js b/src/lib/PreviewUI.js
@@ -23,7 +23,8 @@ import {
     SELECTOR_BOX_PREVIEW_LOGO_DEFAULT,
     SELECTOR_BOX_PREVIEW,
     SELECTOR_NAVIGATION_LEFT,
-    SELECTOR_NAVIGATION_RIGHT
+    SELECTOR_NAVIGATION_RIGHT,
+    SELECTOR_BOX_PREVIEW_CONTENT
 } from './constants';
 
 class PreviewUI {
@@ -45,6 +46,9 @@ class PreviewUI {
     /** @property {Function} - Keydown handler */
     keydownHandler;
 
+    /** @property {HTMLElement} - Preview container element which houses the sidebar and content */
+    previewContainer;
+
     /** @property {ProgressBar} - Progress bar instance */
     progressBar;
 
@@ -59,8 +63,8 @@ class PreviewUI {
             this.progressBar.destroy();
         }
 
-        if (this.contentContainer) {
-            this.contentContainer.removeEventListener('mousemove', this.mousemoveHandler);
+        if (this.previewContainer) {
+            this.previewContainer.removeEventListener('mousemove', this.mousemoveHandler);
         }
 
         if (this.container) {
@@ -109,7 +113,8 @@ class PreviewUI {
         insertTemplate(this.container, shellTemplate);
 
         this.container = this.container.querySelector(SELECTOR_BOX_PREVIEW_CONTAINER);
-        this.contentContainer = this.container.querySelector(SELECTOR_BOX_PREVIEW);
+        this.previewContainer = this.container.querySelector(SELECTOR_BOX_PREVIEW);
+        this.contentContainer = this.container.querySelector(SELECTOR_BOX_PREVIEW_CONTENT);
 
         // Setup the header, buttons, and theme
         if (options.header !== 'none') {
@@ -169,20 +174,20 @@ class PreviewUI {
         // Hide the arrows by default
         leftNavEl.classList.add(CLASS_HIDDEN);
         rightNavEl.classList.add(CLASS_HIDDEN);
-        this.contentContainer.classList.remove(CLASS_BOX_PREVIEW_HAS_NAVIGATION);
+        this.previewContainer.classList.remove(CLASS_BOX_PREVIEW_HAS_NAVIGATION);
 
         leftNavEl.removeEventListener('click', this.leftHandler);
         rightNavEl.removeEventListener('click', this.rightHandler);
-        this.contentContainer.removeEventListener('mousemove', this.mousemoveHandler);
+        this.previewContainer.removeEventListener('mousemove', this.mousemoveHandler);
 
         // Don't show navigation when there is no need
         if (collection.length < 2) {
             return;
         }
 
-        this.contentContainer.classList.add(CLASS_BOX_PREVIEW_HAS_NAVIGATION);
+        this.previewContainer.classList.add(CLASS_BOX_PREVIEW_HAS_NAVIGATION);
 
-        this.contentContainer.addEventListener('mousemove', this.mousemoveHandler);
+        this.previewContainer.addEventListener('mousemove', this.mousemoveHandler);
 
         // Selectively show or hide the navigation arrows
         const index = collection.indexOf(id);
@@ -259,8 +264,8 @@ class PreviewUI {
      * @return {void}
      */
     showLoadingIndicator() {
-        if (this.contentContainer) {
-            this.contentContainer.classList.remove(CLASS_PREVIEW_LOADED);
+        if (this.previewContainer) {
+            this.previewContainer.classList.remove(CLASS_PREVIEW_LOADED);
         }
     }
 
@@ -271,14 +276,14 @@ class PreviewUI {
      * @return {void}
      */
     hideLoadingIndicator() {
-        if (!this.contentContainer) {
+        if (!this.previewContainer) {
             return;
         }
 
-        this.contentContainer.classList.add(CLASS_PREVIEW_LOADED);
+        this.previewContainer.classList.add(CLASS_PREVIEW_LOADED);
 
         // Re-show the cralwer for the next preview since it is hidden in finishLoadingSetup() in BaseViewer.js
-        const crawler = this.contentContainer.querySelector(SELECTOR_BOX_PREVIEW_CRAWLER_WRAPPER);
+        const crawler = this.previewContainer.querySelector(SELECTOR_BOX_PREVIEW_CRAWLER_WRAPPER);
         if (crawler) {
             crawler.classList.remove(CLASS_HIDDEN);
         }
@@ -385,7 +390,7 @@ class PreviewUI {
 
         const headerEl = headerContainerEl.firstElementChild;
         headerEl.className = `${CLASS_BOX_PREVIEW_HEADER} ${CLASS_BOX_PREVIEW_BASE_HEADER}`;
-        this.contentContainer.classList.add(CLASS_BOX_PREVIEW_HAS_HEADER);
+        this.previewContainer.classList.add(CLASS_BOX_PREVIEW_HAS_HEADER);
 
         // Setup theme, default is 'light'
         if (headerTheme === 'dark') {

diff --git a/src/lib/__tests__/Notification-test.js b/src/lib/__tests__/Notification-test.js
@@ -68,7 +68,7 @@ describe('lib/Notification', () => {
             notif.hide();
             notif.show('test');
             assert.equal(notif.messageEl.textContent, 'test');
-            assert.equal(notif.buttonEl.textContent, __('notification_button_default_text'));
+            assert.equal(notif.buttonEl.children[0].nodeName, 'svg');
         });
 
         it('should hide after the timeout', () => {

diff --git a/src/lib/__tests__/PreviewUI-test.html b/src/lib/__tests__/PreviewUI-test.html
@@ -5,5 +5,7 @@
             <img class="bp-is-hidden bp-custom-logo"></img>
         </div>
     </div>
-    <div class="bp"></div>
+    <div class="bp">
+        <div class="bp-content"></div>
+    </div>
 </div>
diff --git a/src/lib/__tests__/PreviewUI-test.js b/src/lib/__tests__/PreviewUI-test.js
@@ -165,17 +165,17 @@ describe('lib/PreviewUI', () => {
             });
 
             it('should add a class if navigation is present', () => {
-                const { contentContainer } = ui;
+                const { previewContainer } = ui;
                 ui.showNavigation('1', ['1']);
-                let isShowingNavigation = contentContainer.classList.contains(
+                let isShowingNavigation = previewContainer.classList.contains(
                     constants.CLASS_BOX_PREVIEW_HAS_NAVIGATION
                 );
                 expect(isShowingNavigation).to.be.false;
                 ui.showNavigation('1', ['1', '2']);
-                isShowingNavigation = contentContainer.classList.contains(constants.CLASS_BOX_PREVIEW_HAS_NAVIGATION);
+                isShowingNavigation = previewContainer.classList.contains(constants.CLASS_BOX_PREVIEW_HAS_NAVIGATION);
                 expect(isShowingNavigation).to.be.true;
                 ui.showNavigation('1', ['1']);
-                isShowingNavigation = contentContainer.classList.contains(constants.CLASS_BOX_PREVIEW_HAS_NAVIGATION);
+                isShowingNavigation = previewContainer.classList.contains(constants.CLASS_BOX_PREVIEW_HAS_NAVIGATION);
                 expect(isShowingNavigation).to.be.false;
             });
         });

diff --git a/src/lib/_boxui.scss b/src/lib/_boxui.scss
@@ -372,8 +372,17 @@
             cursor: pointer;
             font-size: 14px;
             font-weight: bold;
+            margin: 0 7px;
             opacity: 0.6;
             outline: none;
+
+            &.close-btn.default-close-btn {
+                width: 24px;
+
+                path.icon {
+                    fill: $white;
+                }
+            }
         }
     }
 }

diff --git a/src/lib/viewers/BaseViewer.js b/src/lib/viewers/BaseViewer.js
@@ -494,7 +494,7 @@ class BaseViewer extends EventEmitter {
         if (downloadHostToNotify) {
             this.previewUI.notification.show(
                 replacePlaceholders(__('notification_degraded_preview'), [downloadHostToNotify]),
-                __('notification_button_default_text'),
+                null,
                 true
             );
 

diff --git a/src/lib/viewers/doc/DocPreloader.js b/src/lib/viewers/doc/DocPreloader.js
@@ -390,7 +390,7 @@ class DocPreloader extends EventEmitter {
      */
     checkDocumentLoaded() {
         // If document is already loaded, hide the preload and short circuit
-        if (this.previewUI.contentContainer.classList.contains(CLASS_PREVIEW_LOADED)) {
+        if (this.previewUI.previewContainer.classList.contains(CLASS_PREVIEW_LOADED)) {
             this.hidePreload();
             return true;
         }

diff --git a/src/lib/viewers/doc/__tests__/DocPreloader-test.js b/src/lib/viewers/doc/__tests__/DocPreloader-test.js
@@ -31,8 +31,8 @@ describe('lib/viewers/doc/DocPreloader', () => {
         stubs = {};
 
         docPreloader.previewUI = {
-            contentContainer: document.createElement('div'),
-            hideLoadingIndicator: sandbox.stub()
+            hideLoadingIndicator: sandbox.stub(),
+            previewContainer: document.createElement('div')
         };
     });
 
@@ -549,11 +549,11 @@ describe('lib/viewers/doc/DocPreloader', () => {
 
     describe('checkDocumentLoaded()', () => {
         beforeEach(() => {
-            docPreloader.previewUI.contentContainer = document.createElement('div');
+            docPreloader.previewUI.previewContainer = document.createElement('div');
         });
 
         it('should hide preload and return true if container element does have loaded class', () => {
-            docPreloader.previewUI.contentContainer.classList.add(CLASS_PREVIEW_LOADED);
+            docPreloader.previewUI.previewContainer.classList.add(CLASS_PREVIEW_LOADED);
             sandbox.mock(docPreloader).expects('hidePreload');
             expect(docPreloader.checkDocumentLoaded()).to.be.true;
         });