Do not provide share link if (annotation or document) URL is not available on the web #2871

lyzadanger · 2021-01-07T19:06:57Z

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.

With these changes...

Note: For convenience, the screenshots here show both share-single-annotation and share-a-page's-annotations open at the same time. While this isn't broken or impossible, it's probably slightly unusual.

These changes create a new UI path for both the share-single-annotation and share-a-page's-annotations components. When the URI is "shareable," there is no change from before, which looks like this:

If the URI in question is not "shareable," the interfaces will look like this:

To support these changes, component changes were needed (duh), but a few other supporting changes were made as well to the util/annotation-sharing module, and some adjustments to card and panel molecules that I'll elaborate on with inline comments in this PR.

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

In both the share-single-annotation and share-annotations-on-a-page 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", the URI in question needs to have an `http:` or `https:` protocol. In cases where the URI is not "shareable," provide some explanatory text about its non-shareability and don't provide a sharing (bouncer) link. This change is 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. Fixes #2786

lyzadanger · 2021-01-07T19:08:14Z

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


  const copyShareLink = () => {
    try {
-      copyText(shareURI);
+      copyText(shareURI ?? '');


Appeasing the TS gods

Even though it is a bit verbose I would prefer to use a cast here (/** @type {string} */ (shareURI)). This avoids adding an untested path to the code and also makes it more obvious to future readers that this is purely to keep TS happy.

As an aside, this is one area where TypeScript's native syntax is much more succinct (just add an exclamation mark after the expression - shareURI!).

lyzadanger · 2021-01-07T19:08:26Z

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

        <div className="share-annotations-panel">
          <div className="share-annotations-panel__intro">
-            {focusedGroup.type === 'private' ? (
+            {focusedGroup?.type === 'private' ? (


TS again...

I think it would be better to resolve this by changing the conditional to shareURI && uriShareable && focusedGroup even though the last clause is redundant. It generates less code and avoids confusion for the reader about whether focusedGroup can in fact be null in these various cases.

I found an existing issue in TypeScript about this: microsoft/TypeScript#24865.

lyzadanger · 2021-01-07T19:08:59Z

src/sidebar/util/annotation-sharing.js

+ * @param {string} groupID
+ * @return {string}
+ */
+export function getSharingLink(documentURI, groupID) {


This function's logic was moved from the ShareAnnotationsPanel component to here.

lyzadanger · 2021-01-07T19:09:24Z

src/sidebar/util/annotation-sharing.js

 }

 /**
- * Return any defined standalone URI for this `annotation`, preferably the
- * `incontext` URI, but fallback to `html` link if not present.
+ * Return the service-provided sharing URI for an annotation. Prefer the


Just some minor improvements and typing while I was in here.

lyzadanger · 2021-01-07T19:10:02Z

src/sidebar/util/annotation-sharing.js

 * @param {HostConfig} settings
 * @return {boolean}
 */
-export function isShareable(annotation, settings) {
-  return !!(sharingEnabled(settings) && shareURI(annotation));
+export function sharingEnabled(settings) {


Note that we check config (i.e. call this function) to determine whether it should be allowed to share single annotations, but the logic for showing the "share all annotations on this page" is simply "is this first-party?" (tested in AnnotationActionBar).

lyzadanger · 2021-01-07T19:12:10Z

src/styles/mixins/molecules.scss

@@ -20,9 +20,9 @@
 * annotation (thread) cards. Will vertically-space its children. Adds a
 * hover/active intensified box shadow and accounts for "theme-clean" affordances.
 */
-@mixin card {
+@mixin card($rhythm: var.$layout-space) {


The updates to this module's mixins were because the panel--compact mixin was initially designed for the specific "design case" of the share-single-annotation small panel with the text-input field in it, instead of the more "generic" header-and-text layout that is used in the new UI path (not shareable). This fixes it such that the tighter padding and rhythm in the variant that has the input field is the exception rather than the rule.

lyzadanger · 2021-01-07T19:12:50Z

src/styles/sidebar/components/annotation-share-control.scss

  cursor: default;

+  // Hide the bottom border on the panel's header if displaying


This is where the variant with the sharing link overrides the generic panel styling instead of vice-versa.

lyzadanger · 2021-01-07T19:17:51Z

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

@@ -53,7 +53,7 @@ function AnnotationActionBar({
  //  Only authenticated users can flag an annotation, except the annotation's author.
  const showFlagAction =
    !!userProfile.userid && userProfile.userid !== annotation.user;
-  const showShareAction = isShareable(annotation, settings);
+  const showShareAction = sharingEnabled(settings) && shareURI(annotation);


The overall refactor of annotation-sharing logic made it more coherent to have this logic in two discrete functions instead of one. There is some complexity because "is sharing allowed" is not a single question, but, kind of, four:

Should we show the icon for sharing this page's annotations? The logic we use (unchanged) is "Are we first-party?" This is in AnnotationActionBar at present.

Should we show the icon for sharing a single annotation? (This line here). The logic we use (unchanged) is: is there anything in the app's config that says we shouldn't allow annotation sharing, and does this annotation have a valid single-annotation URL in its links?

Which UI should we be providing within the "share all annotations on this page" panel, once it's open? (New). If the page's URI is shareable (i.e. web-accessible), we render the main, pre-existing UI that provides a bouncer link. If not, we render the new UI path explaining why the page's annotations are not shareable (with no link).

Which UI should we be providing within the "share this annotation" panel, once it's open? (New). If the annotation's URI is shareable (i.e. web-accessible), we render the main, pre-existing UI that provides a bouncer link. If not, we render the new UI path explaining why this annotation is not shareable (with no link).

lyzadanger · 2021-01-07T19:19:57Z

src/sidebar/util/annotation-sharing.js

-  }
-  return serviceConfig_.enableShareLinks;
+export function isShareableURI(uri) {
+  return /^http(s?):/.test(uri);


So, this is dead simple. Is it correct?

I opted not to instantiate a URL object here, though we certainly could.

This is fine for the initial use case of not showing these share links when the file is a local PDF file. I think it is reasonable to assume that all shareable URLs use HTTP(S) as the protocol. Not all HTTP(S) URLs are shareable though. A common example is a temporary signed HTTP URL for a PDF file in an S3 bucket (this is used by PDF files in Canvas for example). Handling that I think should be a responsibility of the backend rather than h.

robertknight

My initial thought after looking at this is that the text may be misleading. Where it says "the annotation can't be shared" what it means is "the annotation can't be shared in context". It is still possible (and useful to users) to share the content of the annotation itself and the discussion via a link to the annotation or a link to a notebook/activity page view that shows the annotations.

One possibility would be to show the same UI when the document URL is shareable as when it is not, but change the type of link (eg. show the html link for an annotation instead of the incontext one). However it may be confusing if the link does different things in different situations. Perhaps some explanatory text below the link may be enough to resolve that.

Regarding the code, I noted an instance of commented out code and had some recommendations about working around limitations in TypeScript's ability to infer when a value is non-null.

robertknight · 2021-01-08T09:55:52Z

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

        <div className="share-annotations-panel">
          <div className="share-annotations-panel__intro">
-            {focusedGroup.type === 'private' ? (
+            {focusedGroup?.type === 'private' ? (


I think it would be better to resolve this by changing the conditional to shareURI && uriShareable && focusedGroup even though the last clause is redundant. It generates less code and avoids confusion for the reader about whether focusedGroup can in fact be null in these various cases.

I found an existing issue in TypeScript about this: microsoft/TypeScript#24865.

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

src/sidebar/util/annotation-sharing.js

robertknight · 2021-01-08T10:06:18Z

src/sidebar/util/annotation-sharing.js

-  }
-  return serviceConfig_.enableShareLinks;
+export function isShareableURI(uri) {
+  return /^http(s?):/.test(uri);


This is fine for the initial use case of not showing these share links when the file is a local PDF file. I think it is reasonable to assume that all shareable URLs use HTTP(S) as the protocol. Not all HTTP(S) URLs are shareable though. A common example is a temporary signed HTTP URL for a PDF file in an S3 bucket (this is used by PDF files in Canvas for example). Handling that I think should be a responsibility of the backend rather than h.

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

robertknight · 2021-01-08T11:00:11Z

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


  const copyShareLink = () => {
    try {
-      copyText(shareURI);
+      copyText(shareURI ?? '');


Even though it is a bit verbose I would prefer to use a cast here (/** @type {string} */ (shareURI)). This avoids adding an untested path to the code and also makes it more obvious to future readers that this is purely to keep TS happy.

As an aside, this is one area where TypeScript's native syntax is much more succinct (just add an exclamation mark after the expression - shareURI!).

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

@robertknight I've addressed your code feedback on my working branch. Given that there was a second iteration of how sharing single annotations is handled in these cases, I'm going to open a fresh PR (with your feedback incorporated) for easier navigation.

lyzadanger commented Jan 7, 2021

View reviewed changes

lyzadanger requested a review from robertknight January 7, 2021 19:30

robertknight reviewed Jan 8, 2021

View reviewed changes

lyzadanger closed this Jan 8, 2021

lyzadanger mentioned this pull request Jan 8, 2021

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not provide share link if (annotation or document) URL is not available on the web #2871

Do not provide share link if (annotation or document) URL is not available on the web #2871

lyzadanger commented Jan 7, 2021

lyzadanger Jan 7, 2021

robertknight Jan 8, 2021

lyzadanger Jan 7, 2021

robertknight Jan 8, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

lyzadanger Jan 7, 2021

robertknight Jan 8, 2021

robertknight left a comment •

edited

Loading

robertknight Jan 8, 2021

robertknight Jan 8, 2021

robertknight Jan 8, 2021

lyzadanger commented Jan 8, 2021

		cursor: default;

		// Hide the bottom border on the panel's header if displaying

Do not provide share link if (annotation or document) URL is not available on the web #2871

Do not provide share link if (annotation or document) URL is not available on the web #2871

Conversation

lyzadanger commented Jan 7, 2021

With these changes...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robertknight left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyzadanger commented Jan 8, 2021

robertknight left a comment •

edited

Loading