Provide a UI path for sharing when a page's or annotation's URI is not web-accessible #2874

lyzadanger · 2021-01-08T20:35:46Z

There has been some (understandable) ongoing confusion around share links for single annotations or sets of annotations that pertain to a URI that is not available on the web. Specifically, it's confusing if one annotates a local PDF and then attempts to share a single annotation or a set of annotations on that PDF. Before these changes, the application would happily cook up a bouncer (sharing) link for you, but that link wouldn't do anything useful.

This PR replaces #2871

With these changes...

These changes create a new UI path for both "share this page's annotations" and "share this annotation" when the page's URI or the annotation's URI (respectively) are not web-accessible.

When the page's URI is not web-accessible, using the share-this-page's-annotations button will now result in:

When an annotation's URI is not web-accessible, using the share-this-annotation button will now result in:

The link provided, as explained, will go to the single-annotation view.

The "panel" that the share-this-annotation content shows up in has been slightly widened—from 20em to 24em (with a max-width of 85vw to keep it from getting ungainly on narrow viewports). This accommodates the longer text needed to explain the not-available-in-context wording without risking that this panel on the first annotation in the list could overlap the top bar.

I manually tested that:

When built into the local extension, local PDF files appropriately get the new, unshareable treatment.
When built into the local extension, web-hosted PDF files still allow sharing correctly.

Fixes #2786

klemay · 2021-01-08T21:13:41Z

@lyzadanger nice & elegant solution, IMO! kudos!

robertknight

The UI changes look good. I left a few comments in the implementation. In particular:

It may be worth combining the pageSharingLink and isShareableURI functions
Should we still show the social media share links if the link is an HTML rather than in-context link?
What should the panel show if the document URI is not known yet (eg. because it is a PDF that is still loading)?

robertknight · 2021-01-11T14:21:33Z

src/sidebar/components/share-annotations-panel.js

@@ -95,7 +98,7 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
            ) : (
              <span>
                Anyone using this link may view the annotations in the group{' '}
-                <em>{focusedGroup.name}</em>.
+                <em>{focusedGroup?.name}</em>.


This ? isn't needed any more.

src/sidebar/components/share-annotations-panel.js

src/sidebar/components/annotation-share-control.js

src/sidebar/components/test/annotation-share-control-test.js

src/sidebar/util/annotation-sharing.js

src/sidebar/util/test/annotation-sharing-test.js

src/styles/mixins/molecules.scss

codecov · 2021-01-12T16:43:48Z

Codecov Report

Merging #2874 (14ef85f) into master (bdb5a47) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #2874   +/-   ##
=======================================
  Coverage   97.74%   97.75%           
=======================================
  Files         206      207    +1     
  Lines        7677     7693   +16     
  Branches     1694     1705   +11     
=======================================
+ Hits         7504     7520   +16     
  Misses        173      173

Impacted Files	Coverage Δ
src/sidebar/components/annotation-action-bar.js	`100.00% <100.00%> (ø)`
src/sidebar/components/annotation-share-control.js	`100.00% <100.00%> (ø)`
src/sidebar/components/share-annotations-panel.js	`100.00% <100.00%> (ø)`
src/sidebar/util/annotation-sharing.js	`100.00% <100.00%> (ø)`
src/sidebar/util/typing.js	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bdb5a47...14ef85f. Read the comment docs.

lyzadanger · 2021-01-12T16:44:43Z

@robertknight I've addressed this round of changes, involving:

Change in logic/state structure in ShareAnnotationsPanel. It had become convoluted and wonky in part because I was doing some back-bends to avoid typechecking complaints;
Relatedly, there is a new utility module with a notNull helper for places where typing is missing some clues about context
Pushed some logic about isShareableURI into getPageSharingLink as you suggested
Made isShareableURI case-insensitive
ShareAnnotationsPanel will now show a spinner while loading

lyzadanger · 2021-01-12T16:45:44Z

src/sidebar/components/share-annotations-panel.js

@@ -59,55 +58,70 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
      title={panelTitle}
      panelName={uiConstants.PANEL_SHARE_ANNOTATIONS}
    >
-      {focusedGroup && mainFrame && (
+      {!sharingReady && (


I hope the states are more clear now. There is one top-level branch now: "is shiz loaded enough yet?" and one secondary branch when the answer is "yes": "Do we have a valid share URI?"

This makes sense to me. If it gets significantly more complex in future that it might help to factor out a helper component in the same module.

lyzadanger · 2021-01-12T16:47:35Z

src/styles/sidebar/components/share-annotations-panel.scss

@@ -19,4 +19,12 @@
  &__icon-button {
    @include buttons.button--input;
  }
+
+  // FIXME: Centralize styling of different sizes of Spinner in a reusable


Note this: spinner sizing is a function of font sizing, as it is in ems (which is fine). At some point we should encapsulate some patterns here so that we have the right small set of defined spinner sizes. The spinner at the panel's font size was too small.

lyzadanger · 2021-01-12T16:48:28Z

@robertknight I should point out two things that have not been addressed:

Pulling out analytics code
Showing sharing links when using the html link

Trying to bound the scope of this work, but if you feel passionately on either of these, let me know.

robertknight

LGTM with a few minor comments.

src/sidebar/components/annotation-action-bar.js

src/sidebar/components/annotation-share-control.js

robertknight · 2021-01-12T17:08:20Z

src/sidebar/components/share-annotations-panel.js

@@ -59,55 +58,70 @@ function ShareAnnotationsPanel({ analytics, toastMessenger }) {
      title={panelTitle}
      panelName={uiConstants.PANEL_SHARE_ANNOTATIONS}
    >
-      {focusedGroup && mainFrame && (
+      {!sharingReady && (


This makes sense to me. If it gets significantly more complex in future that it might help to factor out a helper component in the same module.

src/sidebar/util/annotation-sharing.js

robertknight · 2021-01-12T17:14:24Z

src/sidebar/util/test/annotation-sharing-test.js

+        assert.isUndefined(sharingUtil.annotationSharingLink(fakeAnnotation));
+      });
+
+      it('returns `undefined` if no links on annotation', () => {


Can this ever actually happen with the API or will it always set links but possibly leave the object empty?

I do not know offhand.

src/sidebar/util/test/annotation-sharing-test.js

src/sidebar/util/typing.js

Add a typing utility module with a helper function to "cast" the input so that TS understands that it is not null/undefined. This is useful in situations where logic would prevent a reference from being empty but TS can't follow that logic.

In both the share-this-annotation and share-this-page's-annotations components, provide a UI path for when the annotation's URI or the page's URI (respectively) are not web-available. That is, to be "shareable in context", the URI in question needs to have an `http:` or `https:` protocol. For "share this page's annotations" when the page is not web-accessible, explain why the page's annotations can't be shared in context and don't provide a sharing link, as it won't work. For "share this annotation," when the annotation's URI (document) isn't web-accessible, provide some explanatory text about how it can't be shared in-context, but provide a link to the single-annotation view, when available. These changes are intended to avoid confusion when users try to share an annotation or a set of annotations that have been made on a local (e.g. PDF) document. Some SCSS patterns have also been adjusted to accommodate more flexible styling of the share-single-annotation panel. The width of the compact panel pattern has been increased slightly so that the new wording variant for not-shareable-in-context single annotations doesn't risk taking up too much vertical space. Fixes #2786

lyzadanger requested a review from robertknight January 8, 2021 20:35

robertknight reviewed Jan 11, 2021

View reviewed changes

lyzadanger force-pushed the local-sharing branch from c34787a to 77129d5 Compare January 12, 2021 16:41

lyzadanger requested a review from robertknight January 12, 2021 16:44

lyzadanger commented Jan 12, 2021

View reviewed changes

robertknight approved these changes Jan 12, 2021

View reviewed changes

lyzadanger added 2 commits January 12, 2021 13:30

Add typing utility module

faadd9f

Add a typing utility module with a helper function to "cast" the input so that TS understands that it is not null/undefined. This is useful in situations where logic would prevent a reference from being empty but TS can't follow that logic.

Add some details about annotation links

cf990f1

lyzadanger force-pushed the local-sharing branch from 77129d5 to cc0c93a Compare January 12, 2021 18:31

lyzadanger force-pushed the local-sharing branch from cc0c93a to 14ef85f Compare January 13, 2021 13:52

lyzadanger merged commit 53b3321 into master Jan 13, 2021

lyzadanger deleted the local-sharing branch January 13, 2021 14:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a UI path for sharing when a page's or annotation's URI is not web-accessible #2874

Provide a UI path for sharing when a page's or annotation's URI is not web-accessible #2874

lyzadanger commented Jan 8, 2021

klemay commented Jan 8, 2021

robertknight left a comment

robertknight Jan 11, 2021

codecov bot commented Jan 12, 2021 •

edited

Loading

lyzadanger commented Jan 12, 2021

lyzadanger Jan 12, 2021

robertknight Jan 12, 2021

lyzadanger Jan 12, 2021

lyzadanger commented Jan 12, 2021

robertknight left a comment

robertknight Jan 12, 2021

robertknight Jan 12, 2021

lyzadanger Jan 12, 2021

Provide a UI path for sharing when a page's or annotation's URI is not web-accessible #2874

Provide a UI path for sharing when a page's or annotation's URI is not web-accessible #2874

Conversation

lyzadanger commented Jan 8, 2021

With these changes...

klemay commented Jan 8, 2021

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jan 11, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2021 • edited Loading

Codecov Report

lyzadanger commented Jan 12, 2021

lyzadanger Jan 12, 2021

Choose a reason for hiding this comment

robertknight Jan 12, 2021

Choose a reason for hiding this comment

lyzadanger Jan 12, 2021

Choose a reason for hiding this comment

lyzadanger commented Jan 12, 2021

robertknight left a comment

Choose a reason for hiding this comment

robertknight Jan 12, 2021

Choose a reason for hiding this comment

robertknight Jan 12, 2021

Choose a reason for hiding this comment

lyzadanger Jan 12, 2021

Choose a reason for hiding this comment

codecov bot commented Jan 12, 2021 •

edited

Loading