🚀 [Story share] Move share menu to separate bundle (#37179)

* Started to move share menu to bundle * Install amp-social-share only on desktop * Fixed CSS import * Fixed test * Fixed CSS import * Cleaned * Fix tets * Removed console.log, new test * Removed unused import in test * Removed unused import * Fixed dependency checks * Fixed zindeex
ampproject · Dec 13, 2021 · 584f5a6 · 584f5a6
1 parent 9b8d9f4
commit 584f5a6
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 144 deletions.
diff --git a/build-system/compile/bundles.config.extensions.json b/build-system/compile/bundles.config.extensions.json
@@ -885,6 +885,14 @@
       "hasCss": true
     }
   },
+  {
+    "name": "amp-story-share-menu",
+    "version": "0.1",
+    "latestVersion": "0.1",
+    "options": {
+      "hasCss": true
+    }
+  },
   {
     "name": "amp-story-shopping",
     "version": "0.1",

diff --git a/build-system/test-configs/dep-check-config.js b/build-system/test-configs/dep-check-config.js
@@ -264,6 +264,14 @@ exports.rules = [
       'extensions/amp-story-education/0.1/amp-story-education.js->extensions/amp-story/1.0/utils.js',
       'extensions/amp-story-education/0.1/amp-story-education.js->extensions/amp-story/1.0/amp-story-localization-service.js',
 
+      // Story share menu
+      'extensions/amp-story-share-menu/0.1/amp-story-share-menu.js->extensions/amp-story/1.0/utils.js',
+      'extensions/amp-story-share-menu/0.1/amp-story-share-menu.js->extensions/amp-story/1.0/amp-story-share.js',
+      'extensions/amp-story-share-menu/0.1/amp-story-share-menu.js->extensions/amp-story/1.0/amp-story-store-service.js',
+      'extensions/amp-story-share-menu/0.1/amp-story-share-menu.js->extensions/amp-story/1.0/story-analytics.js',
+      'extensions/amp-story/1.0/amp-story.js->extensions/amp-story-share-menu/0.1/amp-story-share-menu.js',
+      'extensions/amp-story-share-menu/0.1/amp-story-share-menu.js->extensions/amp-story/1.0/amp-story-localization-service.js',
+
       // Story Shopping
       'extensions/amp-story-shopping/0.1/amp-story-shopping-config.js->extensions/amp-story/1.0/amp-story-store-service.js',
       'extensions/amp-story-shopping/0.1/amp-story-shopping-config.js->extensions/amp-story/1.0/request-utils.js',

diff --git a/css/Z_INDEX.md b/css/Z_INDEX.md
@@ -37,8 +37,8 @@
 | `.i-amphtml-story-consent`                                                                       | 100005     | [extensions/amp-story/1.0/amp-story-consent.css](/extensions/amp-story/1.0/amp-story-consent.css)                                                                       |
 | `amp-story-access[type=blocking]`                                                                | 100004     | [extensions/amp-story/1.0/amp-story-access.css](/extensions/amp-story/1.0/amp-story-access.css)                                                                         |
 | `.i-amphtml-story-toast`                                                                         | 100004     | [extensions/amp-story/1.0/amp-story.css](/extensions/amp-story/1.0/amp-story.css)                                                                                       |
+| `.i-amphtml-story-share-menu`                                                                    | 100003     | [extensions/amp-story-share-menu/0.1/amp-story-share-menu.css](/extensions/amp-story-share-menu/0.1/amp-story-share-menu.css)                                           |
 | `amp-story-access`                                                                               | 100003     | [extensions/amp-story/1.0/amp-story-access.css](/extensions/amp-story/1.0/amp-story-access.css)                                                                         |
-| `.i-amphtml-story-share-menu`                                                                    | 100003     | [extensions/amp-story/1.0/amp-story-share-menu.css](/extensions/amp-story/1.0/amp-story-share-menu.css)                                                                 |
 | `.i-amphtml-story-has-new-page-notification-container`                                           | 100002     | [extensions/amp-story/1.0/amp-story-system-layer.css](/extensions/amp-story/1.0/amp-story-system-layer.css)                                                             |
 | `amp-story[standalone] .i-amphtml-story-developer-log`                                           | 100002     | [extensions/amp-story/1.0/amp-story.css](/extensions/amp-story/1.0/amp-story.css)                                                                                       |
 | `.i-amphtml-story-button-container`                                                              | 100002     | [extensions/amp-story/1.0/pagination-buttons.css](/extensions/amp-story/1.0/pagination-buttons.css)                                                                     |

diff --git a/...ns/amp-story/1.0/amp-story-share-menu.css → ...y-share-menu/0.1/amp-story-share-menu.css b/...ns/amp-story/1.0/amp-story-share-menu.css → ...y-share-menu/0.1/amp-story-share-menu.css
@@ -1,6 +1,6 @@
 
 
-@import './amp-story-shadow-reset.css';
+@import '../../amp-story/1.0/amp-story-shadow-reset.css';
 @import './amp-story-share.css';
 
 .i-amphtml-story-share-menu {

diff --git a/...ons/amp-story/1.0/amp-story-share-menu.js → ...ry-share-menu/0.1/amp-story-share-menu.js b/...ons/amp-story/1.0/amp-story-share-menu.js → ...ry-share-menu/0.1/amp-story-share-menu.js
@@ -1,80 +1,32 @@
+import {Keys_Enum} from '#core/constants/key-codes';
 import * as Preact from '#core/dom/jsx';
-import {
-  ANALYTICS_TAG_NAME,
-  StoryAnalyticsEvent,
-  getAnalyticsService,
-} from './story-analytics';
+
+import {Services} from '#service';
+import {LocalizedStringId_Enum} from '#service/localization/strings';
+
+import {user} from '#utils/log';
+
+import {CSS} from '../../../build/amp-story-share-menu-0.1.css';
+import {getAmpdoc} from '../../../src/service-helpers';
+import {localize} from '../../amp-story/1.0/amp-story-localization-service';
+import {ShareWidget} from '../../amp-story/1.0/amp-story-share';
 import {
   Action,
   StateProperty,
   UIType,
   getStoreService,
-} from './amp-story-store-service';
-import {CSS} from '../../../build/amp-story-share-menu-1.0.css';
-import {Keys_Enum} from '#core/constants/key-codes';
-import {LocalizedStringId_Enum} from '#service/localization/strings';
-import {Services} from '#service';
-import {ShareWidget} from './amp-story-share';
-import {createShadowRootWithStyle} from './utils';
-import {getAmpdoc} from '../../../src/service-helpers';
-import {localize} from './amp-story-localization-service';
+} from '../../amp-story/1.0/amp-story-store-service';
+import {
+  ANALYTICS_TAG_NAME,
+  StoryAnalyticsEvent,
+  getAnalyticsService,
+} from '../../amp-story/1.0/story-analytics';
+import {createShadowRootWithStyle} from '../../amp-story/1.0/utils';
 
 /** @const {string} Class to toggle the share menu. */
 export const VISIBLE_CLASS = 'i-amphtml-story-share-menu-visible';
 
-/**
- * Quick share template, used as a fallback if native sharing is not supported.
- * @param {!Element} element
- * @param {function(Event)} close
- * @param {?Array<?Element|?string>|?Element|?string|undefined} children
- * @return {!Element}
- */
-const renderForFallbackSharing = (element, close, children) => {
-  return (
-    <div
-      class="i-amphtml-story-share-menu i-amphtml-story-system-reset"
-      aria-hidden="true"
-      role="alert"
-      onClick={(event) => {
-        // Close if click occurred directly on this element.
-        if (event.target === event.currentTarget) {
-          close(event);
-        }
-      }}
-    >
-      <div class="i-amphtml-story-share-menu-container">
-        <button
-          class="i-amphtml-story-share-menu-close-button"
-          aria-label={localize(
-            element,
-            LocalizedStringId_Enum.AMP_STORY_CLOSE_BUTTON_LABEL
-          )}
-          role="button"
-          onClick={close}
-        >
-          &times;
-        </button>
-        {children}
-      </div>
-    </div>
-  );
-};
-
-/**
- * System amp-social-share button template.
- * @return {!Element}
- */
-const renderAmpSocialShareSystemElement = () => {
-  return (
-    <amp-social-share
-      type="system"
-      // TODO(alanorozco): This style attr would be nicer as an object.
-      // We need to enable babel-plugin-jsx-style-object in the testing config
-      // so that we can verify results of style objects.
-      style="visibility:hidden;pointer-events:none;z-index:-1"
-    ></amp-social-share>
-  );
-};
+const TAG = 'amp-story-share-menu';
 
 /**
  * Share menu UI.
@@ -134,15 +86,12 @@ export class ShareMenu {
   }
 
   /**
-   * Builds a hidden amp-social-share button that triggers the native system
-   * sharing UI.
+   * Builds a element used for analytics, since the sharing menu is not rendered.
    * @private
    * @return {!Element}
    */
   buildForSystemSharing_() {
-    this.shareWidget_.loadRequiredExtensions(getAmpdoc(this.parentEl_));
-    this.element_ = renderAmpSocialShareSystemElement();
-    return this.element_;
+    return (this.element_ = <div></div>);
   }
 
   /**
@@ -154,10 +103,11 @@ export class ShareMenu {
     const shareWidgetElement = this.shareWidget_.build(
       getAmpdoc(this.parentEl_)
     );
-    this.element_ = renderForFallbackSharing(
-      this.parentEl_,
-      () => this.close_(),
-      shareWidgetElement
+    this.element_ = this.renderForFallbackSharing_(shareWidgetElement);
+    // TODO(mszylkowski): import '../../amp-social-share/0.1/amp-social-share' when this file is lazy loaded.
+    Services.extensionsFor(this.win_).installExtensionForDoc(
+      getAmpdoc(this.parentEl_),
+      'amp-social-share'
     );
 
     // Only listen for closing when system share is unsupported, since the
@@ -203,7 +153,7 @@ export class ShareMenu {
     if (this.isSystemShareSupported_ && isOpen) {
       // Dispatches a click event on the amp-social-share button to trigger the
       // native system sharing UI. This has to be done upon user interaction.
-      this.element_.dispatchEvent(new Event('click'));
+      this.openSystemShare_();
 
       // There is no way to know when the user dismisses the native system share
       // menu, so we pretend it is closed on the story end, and let the native
@@ -217,7 +167,8 @@ export class ShareMenu {
         this.element_.setAttribute('aria-hidden', !isOpen);
       });
     }
-    this.element_[ANALYTICS_TAG_NAME] = 'amp-story-share-menu';
+
+    this.element_[ANALYTICS_TAG_NAME] = TAG;
     this.analyticsService_.triggerEvent(
       isOpen ? StoryAnalyticsEvent.OPEN : StoryAnalyticsEvent.CLOSE,
       this.element_
@@ -244,4 +195,55 @@ export class ShareMenu {
   close_() {
     this.storeService_.dispatch(Action.TOGGLE_SHARE_MENU, false);
   }
+
+  /**
+   * Opens the sharing dialog of native browsers.
+   * @private
+   */
+  openSystemShare_() {
+    const {navigator} = this.win_;
+    const shareData = {
+      url: Services.documentInfoForDoc(this.parentEl_).canonicalUrl,
+      text: this.win_.document.title,
+    };
+    navigator.share(shareData).catch((e) => {
+      user().warn(TAG, e.message, shareData);
+    });
+  }
+
+  /**
+   * Quick share template, used as a fallback if native sharing is not supported.
+   * @param {?Array<?Element|?string>|?Element|?string|undefined} children
+   * @return {!Element}
+   */
+  renderForFallbackSharing_(children) {
+    return (
+      <div
+        class="i-amphtml-story-share-menu i-amphtml-story-system-reset"
+        aria-hidden="true"
+        role="alert"
+        onClick={(event) => {
+          // Close if click occurred directly on this element.
+          if (event.target === event.currentTarget) {
+            this.close_();
+          }
+        }}
+      >
+        <div class="i-amphtml-story-share-menu-container">
+          <button
+            class="i-amphtml-story-share-menu-close-button"
+            aria-label={localize(
+              this.parentEl_,
+              LocalizedStringId_Enum.AMP_STORY_CLOSE_BUTTON_LABEL
+            )}
+            role="button"
+            onClick={this.close_.bind(this)}
+          >
+            &times;
+          </button>
+          {children}
+        </div>
+      </div>
+    );
+  }
 }
diff --git a/extensions/amp-story/1.0/amp-story-share.css → ...-story-share-menu/0.1/amp-story-share.css b/extensions/amp-story/1.0/amp-story-share.css → ...-story-share-menu/0.1/amp-story-share.css
diff --git a/extensions/amp-story/1.0/amp-story-share.js b/extensions/amp-story/1.0/amp-story-share.js
@@ -17,7 +17,6 @@ import {isObject} from '#core/types';
  * @const {!Object<string, !LocalizedStringId_Enum>}
  */
 const SHARE_PROVIDER_LOCALIZED_STRING_ID = map({
-  'system': LocalizedStringId_Enum.AMP_STORY_SHARING_PROVIDER_NAME_SYSTEM,
   'email': LocalizedStringId_Enum.AMP_STORY_SHARING_PROVIDER_NAME_EMAIL,
   'facebook': LocalizedStringId_Enum.AMP_STORY_SHARING_PROVIDER_NAME_FACEBOOK,
   'line': LocalizedStringId_Enum.AMP_STORY_SHARING_PROVIDER_NAME_LINE,
@@ -96,6 +95,10 @@ function buildProvider(doc, shareType) {
     SHARE_PROVIDER_LOCALIZED_STRING_ID[shareType];
 
   if (!shareProviderLocalizedStringId) {
+    user().warn(
+      'AMP-STORY',
+      `'${shareType}'is not a valid share provider type.`
+    );
     return null;
   }
 
@@ -170,7 +173,6 @@ export class ShareWidget {
       <div class="i-amphtml-story-share-widget">
         <ul class="i-amphtml-story-share-list">
           {this.maybeRenderLinkShareButton_()}
-          <li>{this.maybeRenderSystemShareButton_()}</li>
         </ul>
       </div>
     );
@@ -220,21 +222,6 @@ export class ShareWidget {
     Toast.show(this.storyEl_, buildCopySuccessfulToast(this.win.document, url));
   }
 
-  /**
-   * @return {?Element}
-   * @private
-   */
-  maybeRenderSystemShareButton_() {
-    if (!this.isSystemShareSupported()) {
-      // `amp-social-share` will hide `system` buttons when not supported, but
-      // we also need to avoid adding it for rendering reasons.
-      return null;
-    }
-
-    this.loadRequiredExtensions();
-    return buildProvider(this.win.document, 'system');
-  }
-
   /**
    * NOTE(alanorozco): This is a duplicate of the logic in the
    * `amp-social-share` component.
@@ -256,8 +243,6 @@ export class ShareWidget {
    * @protected
    */
   loadProviders() {
-    this.loadRequiredExtensions();
-
     const shareEl = this.storyEl_.querySelector(
       'amp-story-social-share, amp-story-bookend'
     );
@@ -289,16 +274,6 @@ export class ShareWidget {
       delete params['provider'];
     }
 
-    if (provider == 'system') {
-      user().warn(
-        'AMP-STORY',
-        '`system` is not a valid share provider type. Native sharing is ' +
-          'enabled by default and cannot be turned off.',
-        provider
-      );
-      return;
-    }
-
     const element = buildProvider(
       this.win.document,
       /** @type {string} */ (provider)
@@ -317,13 +292,4 @@ export class ShareWidget {
     // always be last in list
     list.insertBefore(item, list.lastElementChild);
   }
-
-  /**
-   */
-  loadRequiredExtensions() {
-    Services.extensionsFor(this.win).installExtensionForDoc(
-      this.getAmpDoc_(),
-      'amp-social-share'
-    );
-  }
 }
diff --git a/extensions/amp-story/1.0/amp-story.js b/extensions/amp-story/1.0/amp-story.js
@@ -53,7 +53,7 @@ import {LiveStoryManager} from './live-story-manager';
 import {MediaPool, MediaType} from './media-pool';
 import {PaginationButtons} from './pagination-buttons';
 import {Services} from '#service';
-import {ShareMenu} from './amp-story-share-menu';
+import {ShareMenu} from '../../amp-story-share-menu/0.1/amp-story-share-menu';
 import {SwipeXYRecognizer} from '../../../src/gesture-recognizers';
 import {SystemLayer} from './amp-story-system-layer';
 import {renderUnsupportedBrowserLayer} from './amp-story-unsupported-browser-layer';