Merge pull request #1612 from hypothesis/selection-tabs-icons

Update and Simplify `SelectionTabs`, `NewNoteButton` icons, styling

src/sidebar/components/button.js

@@ -24,6 +24,7 @@ function Button({
   icon = '',
   isActive = false,
   onClick = () => null,
+  style = {},
   title,
   useCompactStyle = false,
   useInputStyle = false,
@@ -51,6 +52,7 @@ function Button({
       onClick={onClick}
       aria-pressed={isActive}
       title={title}
+      style={style}
     >
       {icon && <SvgIcon name={icon} className="button__icon" />}
       {buttonText}
@@ -108,6 +110,9 @@ Button.propTypes = {
   /** callback for button clicks */
   onClick: propTypes.func,
 
+  /** optional inline styling  */
+  style: propTypes.object,
+
   /**
    * `title`, used for button `title`, is required unless `buttonText` is present
    */

src/sidebar/components/new-note-btn.js

@@ -8,6 +8,8 @@ const useStore = require('../store/use-store');
 const { applyTheme } = require('../util/theme');
 const { withServices } = require('../util/service-context');
 
+const Button = require('./button');
+
 function NewNoteButton({ $rootScope, settings }) {
   const store = useStore(store => ({
     frames: store.frames(),
@@ -23,13 +25,16 @@ function NewNoteButton({ $rootScope, settings }) {
   };
 
   return (
-    <button
-      style={applyTheme(['ctaBackgroundColor'], settings)}
-      className="new-note__create"
+    <div className="new-note-button">
+      <Button
+        buttonText="New note"
+        icon="add"
         onClick={onNewNoteBtnClick}
-    >
-      + New note
-    </button>
+        style={applyTheme(['ctaBackgroundColor'], settings)}
+        useCompactStyle
+        usePrimaryStyle
+      />
+    </div>
   );
 }
 NewNoteButton.propTypes = {

src/sidebar/components/selection-tabs.js

@@ -6,11 +6,12 @@ const { createElement } = require('preact');
 const { Fragment } = require('preact');
 
 const NewNoteBtn = require('./new-note-btn');
-const sessionUtil = require('../util/session');
 const uiConstants = require('../ui-constants');
 const useStore = require('../store/use-store');
 const { withServices } = require('../util/service-context');
 
+const SvgIcon = require('./svg-icon');
+
 /**
  *  Display name of the tab and annotation count.
  */
@@ -72,7 +73,7 @@ Tab.propTypes = {
  *  Tabbed display of annotations and notes.
  */
 
-function SelectionTabs({ isLoading, settings, session }) {
+function SelectionTabs({ isLoading, settings }) {
   const selectedTab = useStore(store => store.getState().selection.selectedTab);
   const noteCount = useStore(store => store.noteCount());
   const annotationCount = useStore(store => store.annotationCount());
@@ -101,10 +102,6 @@ function SelectionTabs({ isLoading, settings, session }) {
   const showNotesUnavailableMessage =
     selectedTab === uiConstants.TAB_NOTES && noteCount === 0;
 
-  const showSidebarTutorial = sessionUtil.shouldShowSidebarTutorial(
-    session.state
-  );
-
   return (
     <Fragment>
       <div
@@ -146,32 +143,23 @@ function SelectionTabs({ isLoading, settings, session }) {
       {selectedTab === uiConstants.TAB_NOTES &&
         settings.enableExperimentalNewNoteButton && <NewNoteBtn />}
       {!isLoading && (
-        <div className="selection-tabs__empty-message">
+        <div>
           {showNotesUnavailableMessage && (
-            <div className="annotation-unavailable-message">
-              <p className="annotation-unavailable-message__label">
+            <div className="selection-tabs__message">
               There are no page notes in this group.
-                {settings.enableExperimentalNewNoteButton &&
-                  !showSidebarTutorial && (
-                    <div className="annotation-unavailable-message__tutorial">
-                      Create one by clicking the{' '}
-                      <i className="help-icon h-icon-note" /> button.
-                    </div>
-                  )}
-              </p>
             </div>
           )}
           {showAnnotationsUnavailableMessage && (
-            <div className="annotation-unavailable-message">
-              <p className="annotation-unavailable-message__label">
+            <div className="selection-tabs__message">
               There are no annotations in this group.
-                {!showSidebarTutorial && (
-                  <div className="annotation-unavailable-message__tutorial">
+              <br />
               Create one by selecting some text and clicking the{' '}
-                    <i className="help-icon h-icon-annotate" /> button.
-                  </div>
-                )}
-              </p>
+              <SvgIcon
+                name="annotate"
+                inline="true"
+                className="selection-tabs__icon"
+              />{' '}
+              button.
             </div>
           )}
         </div>
@@ -187,9 +175,8 @@ SelectionTabs.propTypes = {
 
   // Injected services.
   settings: propTypes.object.isRequired,
-  session: propTypes.object.isRequired,
 };
 
-SelectionTabs.injectedProps = ['session', 'settings'];
+SelectionTabs.injectedProps = ['settings'];
 
 module.exports = withServices(SelectionTabs);

src/sidebar/components/test/new-note-btn-test.js

@@ -2,6 +2,7 @@
 
 const { mount } = require('enzyme');
 const { createElement } = require('preact');
+const { act } = require('preact/test-utils');
 
 const events = require('../../events');
 const NewNoteButton = require('../new-note-btn');
@@ -49,22 +50,22 @@ describe('NewNoteButton', function() {
     NewNoteButton.$imports.$restore();
   });
 
-  it('creates the component', () => {
+  it("sets a backgroundColor equal to the setting's ctaBackgroundColor color", () => {
     const wrapper = createComponent();
-    assert.include(wrapper.text(), 'New note');
-  });
-
-  it("has a backgroundColor equal to the setting's ctaBackgroundColor color", () => {
-    const wrapper = createComponent().find('button');
     assert.equal(
-      wrapper.prop('style').backgroundColor,
+      wrapper.find('Button').prop('style').backgroundColor,
       fakeSettings.branding.ctaBackgroundColor
     );
   });
 
   it('should broadcast BEFORE_ANNOTATION_CREATED event when the new note button is clicked', () => {
     const wrapper = createComponent();
-    wrapper.find('button').simulate('click');
+    act(() => {
+      wrapper
+        .find('Button')
+        .props()
+        .onClick();
+    });
     const topLevelFrame = fakeStore.frames().find(f => !f.id);
     assert.calledWith(
       fakeRootScope.$broadcast,

src/sidebar/components/test/selection-tabs-test.js

@@ -9,7 +9,6 @@ const mockImportedComponents = require('./mock-imported-components');
 
 describe('SelectionTabs', function() {
   // mock services
-  let fakeSession;
   let fakeSettings;
   let fakeStore;
 
@@ -20,23 +19,11 @@ describe('SelectionTabs', function() {
 
   function createComponent(props) {
     return mount(
-      <SelectionTabs
-        session={fakeSession}
-        settings={fakeSettings}
-        {...defaultProps}
-        {...props}
-      />
+      <SelectionTabs settings={fakeSettings} {...defaultProps} {...props} />
     );
   }
 
   beforeEach(() => {
-    fakeSession = {
-      state: {
-        preferences: {
-          show_sidebar_tutorial: false,
-        },
-      },
-    };
     fakeSettings = {
       enableExperimentalNewNoteButton: false,
     };
@@ -65,7 +52,7 @@ describe('SelectionTabs', function() {
   });
 
   const unavailableMessage = wrapper =>
-    wrapper.find('.annotation-unavailable-message__label').text();
+    wrapper.find('.selection-tabs__message').text();
 
   context('displays selection tabs and counts', function() {
     it('should display the tabs and counts of annotations and notes', function() {
@@ -172,7 +159,7 @@ describe('SelectionTabs', function() {
       const wrapper = createComponent({
         isLoading: true,
       });
-      assert.isFalse(wrapper.exists('.annotation-unavailable-message__label'));
+      assert.isFalse(wrapper.exists('.selection-tabs__message'));
     });
 
     it('should not display the longer version of the no annotations message when there are no annotations and isWaitingToAnchorAnnotations is true', function() {
@@ -181,7 +168,7 @@ describe('SelectionTabs', function() {
       const wrapper = createComponent({
         isLoading: false,
       });
-      assert.isFalse(wrapper.exists('.annotation-unavailable-message__label'));
+      assert.isFalse(wrapper.exists('.selection-tabs__message'));
     });
 
     it('should display the longer version of the no notes message when there are no notes', function() {
@@ -196,24 +183,6 @@ describe('SelectionTabs', function() {
       );
     });
 
-    it('should display the prompt to create a note when there are no notes and enableExperimentalNewNoteButton is true', function() {
-      fakeSettings.enableExperimentalNewNoteButton = true;
-      fakeStore.getState.returns({
-        selection: { selectedTab: uiConstants.TAB_NOTES },
-      });
-      fakeStore.noteCount.returns(0);
-      const wrapper = createComponent({});
-      assert.include(
-        wrapper.find('.annotation-unavailable-message__tutorial').text(),
-        'Create one by clicking the'
-      );
-      assert.isTrue(
-        wrapper
-          .find('.annotation-unavailable-message__tutorial i')
-          .hasClass('h-icon-note')
-      );
-    });
-
     it('should display the longer version of the no annotations message when there are no annotations', function() {
       fakeStore.annotationCount.returns(0);
       const wrapper = createComponent({});
@@ -222,49 +191,9 @@ describe('SelectionTabs', function() {
         'There are no annotations in this group.'
       );
       assert.include(
-        wrapper.find('.annotation-unavailable-message__tutorial').text(),
-        'Create one by selecting some text and clicking the'
-      );
-      assert.isTrue(
-        wrapper
-          .find('.annotation-unavailable-message__tutorial i')
-          .hasClass('h-icon-annotate')
-      );
-    });
-
-    context('when the sidebar tutorial is displayed', function() {
-      it('should display the shorter version of the no notes message when there are no notes', function() {
-        fakeSession.state.preferences.show_sidebar_tutorial = true;
-        fakeStore.getState.returns({
-          selection: { selectedTab: uiConstants.TAB_NOTES },
-        });
-        fakeStore.noteCount.returns(0);
-        const wrapper = createComponent({});
-
-        const msg = unavailableMessage(wrapper);
-
-        assert.include(msg, 'There are no page notes in this group.');
-        assert.notInclude(msg, 'Create one by clicking the');
-        assert.notInclude(
-          msg,
-          'Create one by selecting some text and clicking the'
-        );
-      });
-
-      it('should display the shorter version of the no annotations message when there are no annotations', function() {
-        fakeSession.state.preferences.show_sidebar_tutorial = true;
-        fakeStore.annotationCount.returns(0);
-        const wrapper = createComponent({});
-
-        const msg = unavailableMessage(wrapper);
-
-        assert.include(msg, 'There are no annotations in this group.');
-        assert.notInclude(msg, 'Create one by clicking the');
-        assert.notInclude(
-          msg,
+        unavailableMessage(wrapper),
         'Create one by selecting some text and clicking the'
       );
     });
   });
-  });
 });

src/styles/sidebar/components/new-note.scss

 @use "../../variables" as var;
 
-.new-note__create {
-  background-color: var.$grey-mid;
-  border: none;
-  border-radius: 3px;
-  color: #fff;
+.new-note-button {
   display: flex;
-  font-weight: 500;
-  margin-left: auto;
-  margin-right: 14px;
-  margin-bottom: 10px;
-  text-align: center;
+  justify-content: flex-end;
+  margin: 0 1em 1em 0;
 }

src/styles/sidebar/components/selection-tabs.scss

@@ -48,3 +48,17 @@
 .selection-tabs__type--orphan {
   margin-left: -5px;
 }
+
+.selection-tabs__message {
+  border: 1px solid var.$grey-3;
+  padding: 3em;
+  text-align: center;
+}
+
+.selection-tabs__icon {
+  width: 12px;
+  height: 12px;
+  margin-right: 1px;
+  margin-bottom: -1px; // Pull the icon a little toward the baseline
+  color: var.$grey-5;
+}