Refactor focus-annotations functionality

35e92333 · Lyza Danger Gardner · 0374e506 · 35e92333 · 35e92333 · 35e92333
Commit 35e92333 authored Apr 20, 2020 by Lyza Danger Gardner
9 changed files
--- a/src/sidebar/components/annotation-quote.js
+++ b/src/sidebar/components/annotation-quote.js
@@ -11,7 +11,7 @@ import Excerpt from './excerpt';
 /**
 * Display the selected text from the document associated with an annotation.
 */
-function AnnotationQuote({ annotation, settings = {} }) {
+function AnnotationQuote({ annotation, isFocused, settings = {} }) {
  // The language for the quote may be different than the client's UI (set by
  // `<html lang="...">`).
  //
@@ -24,10 +24,10 @@ function AnnotationQuote({ annotation, settings = {} }) {

  return (
    <section
-      className={classnames(
-        'annotation-quote',
-        isOrphan(annotation) && 'is-orphan'
-      )}
+      className={classnames('annotation-quote', {
+        'is-orphan': isOrphan(annotation),
+        'is-focused': isFocused,
+      })}
    >
      <Excerpt
        collapsedHeight={35}
@@ -49,7 +49,8 @@ function AnnotationQuote({ annotation, settings = {} }) {

 AnnotationQuote.propTypes = {
  annotation: propTypes.object.isRequired,
-
+  /** Is this annotation currently focused? */
+  isFocused: propTypes.bool,
  // Used for theming.
  settings: propTypes.object,
 };

--- a/src/sidebar/components/annotation.js
+++ b/src/sidebar/components/annotation.js
@@ -27,6 +27,9 @@ function Annotation({
  toastMessenger,
 }) {
  const createDraft = useStore(store => store.createDraft);
+  const isFocused = useStore(store =>
+    store.isAnnotationFocused(annotation.$tag)
+  );
  const setCollapsed = useStore(store => store.setCollapsed);

  // An annotation will have a draft if it is being edited
@@ -90,6 +93,7 @@ function Annotation({
      className={classnames('annotation', {
        'annotation--reply': isReply(annotation),
        'is-collapsed': threadIsCollapsed,
+        'is-focused': isFocused,
      })}
      onKeyDown={onKeyDown}
    >
@@ -101,7 +105,9 @@ function Annotation({
        threadIsCollapsed={threadIsCollapsed}
      />

-      {hasQuote && <AnnotationQuote annotation={annotation} />}
+      {hasQuote && (
+        <AnnotationQuote annotation={annotation} isFocused={isFocused} />
+      )}

      {!isCollapsedReply && (
        <AnnotationBody

--- a/src/sidebar/components/test/annotation-quote-test.js
+++ b/src/sidebar/components/test/annotation-quote-test.js
@@ -14,7 +14,12 @@ describe('AnnotationQuote', () => {

  function createQuote(props) {
    return mount(
-      <AnnotationQuote annotation={fakeAnnotation} settings={{}} {...props} />
+      <AnnotationQuote
+        annotation={fakeAnnotation}
+        isFocused={false}
+        settings={{}}
+        {...props}
+      />
    );
  }


--- a/src/sidebar/components/test/annotation-test.js
+++ b/src/sidebar/components/test/annotation-test.js
@@ -69,6 +69,7 @@ describe('Annotation', () => {
      getGroup: sinon.stub().returns({
        type: 'private',
      }),
+      isAnnotationFocused: sinon.stub().returns(false),
      isSavingAnnotation: sinon.stub().returns(false),
      profile: sinon.stub().returns({ userid: 'acct:foo@bar.com' }),
      setCollapsed: sinon.stub(),
@@ -97,6 +98,14 @@ describe('Annotation', () => {
      assert.isFalse(annot.hasClass('is-collapsed'));
    });

+    it('applies a focused class if annotation is focused', () => {
+      fakeStore.isAnnotationFocused.returns(true);
+      const wrapper = createComponent({ threadIsCollapsed: false });
+      const annot = wrapper.find('.annotation');
+
+      assert.isTrue(annot.hasClass('is-focused'));
+    });
+
    it('should assign a collapsed class if the annotation thread is collapsed', () => {
      const wrapper = createComponent({ threadIsCollapsed: true });
      const annot = wrapper.find('.annotation');
@@ -114,6 +123,14 @@ describe('Annotation', () => {
      assert.isTrue(quote.exists());
    });

+    it('sets the quote to "focused" if annotation is currently focused', () => {
+      fakeStore.isAnnotationFocused.returns(true);
+      fakeMetadata.quote.returns('quote');
+      const wrapper = createComponent();
+
+      assert.isTrue(wrapper.find('AnnotationQuote').props().isFocused);
+    });
+
    it('does not render quote if annotation does not have a quote', () => {
      fakeMetadata.quote.returns(null);


--- a/src/sidebar/services/frame-sync.js
+++ b/src/sidebar/services/frame-sync.js
@@ -235,14 +235,15 @@ export default function FrameSync($rootScope, $window, store, bridge) {
  };

  /**
-   * Focus annotations with the given tags.
+   * Focus annotations with the given $tags.
   *
   * This is used to indicate the highlight in the document that corresponds to
   * a given annotation in the sidebar.
   *
-   * @param {string[]} tags
+   * @param {string[]} tags - annotation $tags
   */
  this.focusAnnotations = function (tags) {
+    store.focusAnnotations(tags);
    bridge.call('focusAnnotations', tags);
  };


--- a/src/sidebar/services/test/frame-sync-test.js
+++ b/src/sidebar/services/test/frame-sync-test.js
@@ -401,4 +401,22 @@ describe('sidebar/services/frame-sync', function () {
      assert.calledWith(fakeBridge.call, 'setVisibleHighlights');
    });
  });
+
+  describe('when annotations are focused in the sidebar', () => {
+    it('should update the focused annotations in the store', () => {
+      frameSync.focusAnnotations(['a1', 'a2']);
+      assert.calledWith(
+        fakeStore.focusAnnotations,
+        sinon.match.array.deepEquals(['a1', 'a2'])
+      );
+    });
+    it('notify the host page', () => {
+      frameSync.focusAnnotations([1, 2]);
+      assert.calledWith(
+        fakeBridge.call,
+        'focusAnnotations',
+        sinon.match.array.deepEquals([1, 2])
+      );
+    });
+  });
 });
--- a/src/sidebar/store/modules/selection.js
+++ b/src/sidebar/store/modules/selection.js
@@ -92,8 +92,8 @@ const setTab = (selectedTab, newTab) => {

 function init(settings) {
  return {
-    // Contains a map of annotation tag:true pairs.
-    focusedAnnotationMap: null,
+    // An array of annotation `$tag`s representing annotations that are focused
+    focusedAnnotations: [],

    // Contains a map of annotation id:true pairs.
    selectedAnnotationMap: initialSelection(settings),
@@ -153,7 +153,7 @@ const update = {
  },

  FOCUS_ANNOTATIONS: function (state, action) {
-    return { focusedAnnotationMap: action.focused };
+    return { focusedAnnotations: [...action.focusedTags] };
  },

  SET_FOCUS_MODE_FOCUSED: function (state, action) {
@@ -318,7 +318,7 @@ function setForceVisible(id, visible) {
 function focusAnnotations(tags) {
  return {
    type: actions.FOCUS_ANNOTATIONS,
-    focused: freeze(toSet(tags)),
+    focusedTags: tags,
  };
 }

@@ -395,6 +395,13 @@ function setSortKey(key) {
  };
 }

+/** Is the annotation referenced by `$tag` currently focused? */
+function isAnnotationFocused(state, $tag) {
+  return state.selection.focusedAnnotations.some(
+    focusedAnn => $tag && focusedAnn === $tag
+  );
+}
+
 /**
 * Returns true if the annotation with the given `id` is selected.
 */
@@ -542,6 +549,7 @@ export default {
    focusModeHasUser,
    focusModeUserId,
    focusModeUserPrettyName,
+    isAnnotationFocused,
    isAnnotationSelected,
    getFirstSelectedAnnotationId,
    getSelectedAnnotationMap,

--- a/src/sidebar/store/modules/test/selection-test.js
+++ b/src/sidebar/store/modules/test/selection-test.js
@@ -43,28 +43,22 @@ describe('sidebar/store/modules/selection', () => {
  });

  describe('focusAnnotations()', function () {
-    it('adds the passed annotations to the focusedAnnotationMap', function () {
+    it('adds the provided annotation IDs to the focused annotations', function () {
      store.focusAnnotations([1, 2, 3]);
-      assert.deepEqual(getSelectionState().focusedAnnotationMap, {
-        1: true,
-        2: true,
-        3: true,
-      });
+      assert.deepEqual(getSelectionState().focusedAnnotations, [1, 2, 3]);
    });

-    it('replaces any annotations originally in the map', function () {
+    it('replaces any other focused annotation IDs', function () {
      store.focusAnnotations([1]);
      store.focusAnnotations([2, 3]);
-      assert.deepEqual(getSelectionState().focusedAnnotationMap, {
-        2: true,
-        3: true,
-      });
+      assert.deepEqual(getSelectionState().focusedAnnotations, [2, 3]);
    });

-    it('nulls the map if no annotations are focused', function () {
+    it('sets focused annotations to an empty array if no IDs provided', function () {
      store.focusAnnotations([1]);
      store.focusAnnotations([]);
-      assert.isNull(getSelectionState().focusedAnnotationMap);
+      assert.isArray(getSelectionState().focusedAnnotations);
+      assert.isEmpty(getSelectionState().focusedAnnotations);
    });
  });

@@ -368,6 +362,17 @@ describe('sidebar/store/modules/selection', () => {
    });
  });

+  describe('isAnnotationFocused', () => {
+    it('returns true if the provided annotation ID is in the set of focused annotations', () => {
+      store.focusAnnotations([1, 2]);
+      assert.isTrue(store.isAnnotationFocused(2));
+    });
+
+    it('returns false if the provided annotation ID is not in the set of focused annotations', () => {
+      assert.isFalse(store.isAnnotationFocused(2));
+    });
+  });
+
  describe('selectTab()', function () {
    it('sets the selected tab', function () {
      store.selectTab(uiConstants.TAB_ANNOTATIONS);

--- a/src/styles/sidebar/components/annotation-quote.scss
+++ b/src/styles/sidebar/components/annotation-quote.scss
@@ -2,13 +2,12 @@

 .annotation-quote {
  margin: 1em 0;
-}

-.annotation-quote.is-orphan {
+  &.is-orphan {
    text-decoration: line-through;
-}
+  }

-.annotation-quote__quote {
+  &__quote {
    @include var.font-normal;

    border-left: 3px solid var.$grey-3;
@@ -21,4 +20,9 @@

    // Prevent long URLs etc. in quote causing overflow
    overflow-wrap: break-word;
+  }
+
+  &.is-focused &__quote {
+    border-left: var.$highlight 3px solid;
+  }
 }