Added close button to the notebook

The sidebar component directs the opening of the notebook. This is for two reasons:

* it's a little bit easier to save the UI state of the current sidebar
  (immediately before the notebook is opened), in case in the future we
  want to restored it.

* to avoid adding additional events, like `show/hideSidebarControls`,
  that probably would be needed for granular control of the sidebar, in
  case the notebook would be directing the process of closing/re-opening
  the sidebar.

src/annotator/notebook.js

@@ -3,6 +3,9 @@ import { createSidebarConfig } from './config/sidebar';
 import { createShadowRoot } from './util/shadow-root';
 import { render } from 'preact';
 
+// FIXME: use the button from the frontend shared package once this is stable.
+import Button from '../sidebar/components/Button';
+
 /**
  * Create the iframe that will load the notebook application.
  *
@@ -33,7 +36,9 @@ export default class Notebook extends Delegator {
     super(element, config);
     this.frame = null;
 
+    /** @type {null|string} */
     this._groupId = null;
+    /** @type {null|string} */
     this._prevGroupId = null;
 
     /**
@@ -56,8 +61,7 @@ export default class Notebook extends Delegator {
       this._groupId = groupId;
       this.open();
     });
-    this.subscribe('closeNotebook', () => this.close());
-    // If the sidebar has opened, get out of the way
+    // Defensive programming: if the sidebar has opened, get out of the way
     this.subscribe('sidebarOpened', () => this.close());
   }
 
@@ -106,6 +110,25 @@ export default class Notebook extends Delegator {
     this.container.className = 'notebook-outer';
     shadowRoot.appendChild(this.container);
 
+    render(
+      <div className="Notebook__controller-bar">
+        <Button
+          icon="cancel"
+          className="Notebook__close-button"
+          buttonText="Close"
+          title="Close the Notebook"
+          onClick={event => {
+            // Guest 'component' captures all click events in the host page and opens the sidebar.
+            // We stop the propagation of the event to prevent the sidebar to be opened.
+            event.stopPropagation();
+            this.close();
+            this.publish('closeNotebook');
+          }}
+        />
+      </div>,
+      this.container
+    );
+
     return this.container;
   }
 }

src/annotator/sidebar.js

@@ -237,13 +237,14 @@ export default class Sidebar extends Delegator {
 
     // Re-publish the crossframe event so that anything extending Delegator
     // can subscribe to it (without need for crossframe)
-    this.guest.crossframe.on('openNotebook', groupId => {
-      this.close();
+    this.guest.crossframe.on('openNotebook', (
+      /** @type {string} */ groupId
+    ) => {
+      this.hide();
       this.publish('openNotebook', [groupId]);
     });
-    this.guest.crossframe.on('closeNotebook', () => {
-      this.open();
-      this.publish('closeNotebook');
+    this.subscribe('closeNotebook', () => {
+      this.show();
     });
 
     const eventHandlers = [
@@ -472,4 +473,22 @@ export default class Sidebar extends Delegator {
   setAllVisibleHighlights(shouldShowHighlights) {
     this.guest.crossframe.call('setVisibleHighlights', shouldShowHighlights);
   }
+
+  /**
+   * Shows the sidebar's controls
+   */
+  show() {
+    if (this.iframeContainer) {
+      this.iframeContainer.classList.remove('is-hidden');
+    }
+  }
+
+  /**
+   * Hides the sidebar's controls
+   */
+  hide() {
+    if (this.iframeContainer) {
+      this.iframeContainer.classList.add('is-hidden');
+    }
+  }
 }

src/annotator/test/notebook-test.js

@@ -114,22 +114,27 @@ describe('Notebook', () => {
     });
   });
 
-  describe('responding to events', () => {
-    it('opens on `openNotebook`', () => {
+  describe('responding to user input', () => {
+    it('closes the notebook when close button clicked', () => {
       const notebook = createNotebook();
 
-      notebook.publish('openNotebook');
+      notebook.open();
 
-      assert.equal(notebook.container.style.display, '');
+      const button = notebook.container.getElementsByClassName(
+        'Notebook__close-button'
+      )[0];
+      button.click();
+      assert.equal(notebook.container.style.display, 'none');
     });
+  });
 
-    it('closes on `closeNotebook`', () => {
+  describe('responding to events', () => {
+    it('opens on `openNotebook`', () => {
       const notebook = createNotebook();
 
-      notebook.open();
-      notebook.publish('closeNotebook');
+      notebook.publish('openNotebook');
 
-      assert.equal(notebook.container.style.display, 'none');
+      assert.equal(notebook.container.style.display, '');
     });
 
     it('closes on "sidebarOpened"', () => {

src/annotator/test/sidebar-test.js

@@ -278,29 +278,29 @@ describe('Sidebar', () => {
         assert.called(target);
       }));
 
-    describe('on "openNotebook" event', () => {
-      it('hides itself and republishes the event', () => {
+    describe('on "openNotebook" crossframe event', () => {
+      it('hides the sidebar', () => {
         const sidebar = createSidebar();
+        sinon.stub(sidebar, 'hide').callThrough();
         sinon.stub(sidebar, 'publish');
-        sinon.stub(sidebar, 'close');
         emitEvent('openNotebook', 'mygroup');
         assert.calledWith(
           sidebar.publish,
           'openNotebook',
           sinon.match(['mygroup'])
         );
-        assert.calledOnce(sidebar.close);
+        assert.calledOnce(sidebar.hide);
+        assert.notEqual(sidebar.iframeContainer.style.visibility, 'hidden');
       });
     });
 
-    describe('on "closeNotebook" event', () => {
-      it('opens itself and republishes the event', () => {
+    describe('on "closeNotebook" internal event', () => {
+      it('shows the sidebar', () => {
         const sidebar = createSidebar();
-        sinon.stub(sidebar, 'publish');
-        sinon.stub(sidebar, 'open');
-        emitEvent('closeNotebook');
-        assert.calledWith(sidebar.publish, 'closeNotebook');
-        assert.calledOnce(sidebar.open);
+        sinon.stub(sidebar, 'show').callThrough();
+        sidebar.publish('closeNotebook');
+        assert.calledOnce(sidebar.show);
+        assert.equal(sidebar.iframeContainer.style.visibility, '');
       });
     });
 

src/styles/annotator/annotator.scss

@@ -47,6 +47,11 @@
   user-select: none;
   -webkit-tap-highlight-color: rgba(255, 255, 255, 0);
 
+  &.is-hidden {
+    visibility: hidden;
+    transition: visibility var.$annotator-timing--sidebar-collapse-transition;
+  }
+
   &.annotator-collapsed {
     margin-left: 0;
 

src/styles/annotator/notebook.scss

 @use '../variables' as var;
 @use '../mixins/molecules';
+@use "../mixins/buttons";
 
 .notebook-outer {
   box-sizing: border-box;
   position: fixed;
+  // This large zIndex is used to bring the notebook to the front, so it is not
+  // hidden by other elements from the host page. It is the same value used by
+  // the sidebar
+  z-index: 2147483647;
   top: 0;
   left: 0;
   width: 100vw;
   height: 100vh;
   padding: var.$layout-space;
-  // Leave explicitly the right amount of room for closed-sidebar affordances
-  padding-right: var.$annotator-toolbar-width + 5px;
 
   &.is-open {
     // TBD: Actual opacity/overlay we'd like to use
@@ -24,4 +27,30 @@
   padding: 0;
   width: 100%;
   height: 100%;
+
+  // FIXME: these properties produce a visual break between the Notebook__controller-bar and the iframe.
+  // A better approach would be if the Notebook__controller-bar and iframe could be children of notebook-inner.
+  border: none;
+  border-top-left-radius: 0;
+  border-top-right-radius: 0;
+}
+
+// This container element has the purpose of pushing children to the right side.
+.Notebook__controller-bar {
+  font-size: var.$font-size--large;
+  display: flex;
+  justify-content: flex-end;
+  background-color: var.$grey-2;
+  padding: var.$layout-space--xsmall;
+
+  // FIXME: these properties emulates as if the Notebook__controller-bar would be part of the iframe.
+  border-top-left-radius: var.$border-radius;
+  border-top-right-radius: var.$border-radius;
+}
+
+.Notebook__close-button {
+  @include buttons.button--labeled(
+    $background-color: var.$grey-2,
+    $active-background-color: var.$grey-3
+  );
 }

src/styles/sidebar/components/HypothesisApp.scss

@@ -24,9 +24,9 @@
   // FIXME: Temporary affordance for the Notebook view to override some
   // layout spacing that assumes the presence of the top bar
   &--notebook {
-    padding: var.$layout-space;
+    padding: 0 var.$layout-space;
     @include responsive.respond-to(tablets desktops) {
-      padding: var.$layout-space--xlarge;
+      padding: 0 var.$layout-space--xlarge;
     }
   }
 }