✨Bento `amp-soundcloud` by AnuragVasanwala · Pull Request #34828 · ampproject/amphtml

AnuragVasanwala · 2021-06-11T10:35:53Z

This PR adds a bento component for amp-soundcloud.

Fixes #34734

into bento/amp-soundcloud

CLAassistant · 2021-06-11T10:35:57Z

All committers have signed the CLA.

amp-owners-bot · 2021-06-11T10:36:01Z

Hey @ampproject/wg-caching! These files were changed:

extensions/amp-soundcloud/1.0/test/validator-amp-soundcloud.html
extensions/amp-soundcloud/1.0/test/validator-amp-soundcloud.out
extensions/amp-soundcloud/validator-amp-soundcloud.protoascii

extensions/amp-soundcloud/1.0/base-element.js

caroqliu

This is a great start and almost there. I had a few comments and suggestions, but otherwise ready to go. 👍 👍 👍

build-system/compile/bundles.config.extensions.json

extensions/amp-soundcloud/1.0/amp-soundcloud.js

extensions/amp-soundcloud/1.0/base-element.js

extensions/amp-soundcloud/1.0/component.js

extensions/amp-soundcloud/1.0/converter.js

extensions/amp-soundcloud/1.0/storybook/Basic.js

extensions/amp-soundcloud/1.0/test/validator-amp-soundcloud.html

extensions/amp-soundcloud/1.0/amp-soundcloud.css

Bento components are required to be R1. We have an equivalent static getPreconnects(element) in R1 designed to take care of this. Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

ampproject#34838 Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

No need to override `init` if no changes were made to it except calling parent class's `init`. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

…children` Since `passthrough` gives a `<slot>` reference to the children prop in all cases, let's make this passthroughNonEmpty -- which will only give a `<slot>` if there are children in the given markup. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

into bento/amp-soundcloud

No need to explicitly specify boolean `true` for conditional block. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

`height`, `heights`, `media`, `sizes`, and `width` are not needed to pass through as these attributes are specific to AMP's layout system and handled by the runtime. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

`height`, `heights`, `media`, `sizes`, and `width` are not needed to pass through as these attributes are specific to AMP's layout system and handled by the runtime. Thus they do not need to be specify in typedef. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

caroqliu

For some reason I'm not able to comment on the comment thread on IframeEmbed -- there's not currently a way to execute the unmount logic in the IframeEmbed. We can add onMount/onUnmount props to that component so that the Soundcloud component can pass these in, though the current approach is fine as-is as well.

One last recommendation - I noticed that there are unit tests for the Preact layer but not the AMP layer. Please also add extensions/amp-soundcloud/1.0/test/test-amp-soundcloud.js. It can have similar test cases to the one in 0.1, though the iframe reference will come from shadow instead (see amp-instagram unit tests for an example of waiting for render and retrieving the iframe)

build-system/compile/bundles.config.extensions.json

extensions/amp-soundcloud/1.0/test/validator-amp-soundcloud.out

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

caroqliu · 2021-06-17T13:57:59Z

extensions/amp-soundcloud/1.0/component.js

+  useEffect(() => {
+    /** Unmount Procedure */
+    return () => {
+      // Pause widget
+      iframeRef.current?.contentWindow?./*OK*/ postMessage(
+        JSON.stringify(dict({'method': 'pause'})),
+        'https://w.soundcloud.com'
+      );
+
+      // Release iframe resources
+      iframeRef.current = null;
+    };
+  }, []);


@jridgewell Should this section be implemented via BaseElement['unloadOnPause'] or by providing a pause API? Asking based on this logic here, not seeing good examples of how/when to use:

amphtml/src/preact/base-element.js

Lines 846 to 856 in fcc6528

/** @override */

pauseCallback() {

const Ctor = this.constructor;

if (Ctor['unloadOnPause']) {

this.mutateProps(dict({'loading': Loading.UNLOAD}));

this.resetLoading_ = true;

} else {

const {currentRef_: api} = this;

api?.['pause']?.();

}

}

extensions/amp-soundcloud/1.0/base-element.js

antiphoton

LGTM validator files

caroqliu · 2021-06-23T13:25:31Z

Looks like CI is stuck on the lint phase -- please run amp lint --local_changes --fix to resolve.

caroqliu · 2021-06-23T14:43:52Z

It looks like it is still having trouble on the lint step: https://app.circleci.com/pipelines/github/ampproject/amphtml/11890/workflows/5e9b2c18-36fd-4d4e-aa63-677a7efe6bc1/jobs/188651 Have you tried merging with main -- we recently introduced aliased imports (i.e. ../../../src/preact -> #preact) and that is likely what it is wanting to see.

AnuragVasanwala · 2021-06-23T16:03:45Z

Thank you so much to draw attention 👍🏻
I pulled recent updates and fixed aliased imports.
I have also logged in to CircleCI dashboard for the insights to check and solve bug, if any.

AnuragVasanwala · 2021-06-24T06:04:32Z

I am not sure why GitHub Actions / build (windows-latest) (pull_request) is failing. I am looking into the Guthub Actions. Please let me know if any necessary steps are needed from my side.

caroqliu · 2021-06-24T13:15:15Z

I am not sure why GitHub Actions / build (windows-latest) (pull_request) is failing. I am looking into the Guthub Actions. Please let me know if any necessary steps are needed from my side.

It looks like an issue unrelated to your PR. I triggered a rerun and will keep an eye on it today so we can get this merged. :)

* Initial Commit * Property mappings and Cleanup * Minor fixes, Formatting and Comments * More readable variable name * Test Cases for Bento * Updated test-cases and Storybook * Added base CSS File * Bug Fix: Layout System and Unit * Parent element changed from <div> to <> * Type correction for 'visual' parameter * Bug fix for color parameter in storybook * Update extensions/amp-soundcloud/1.0/base-element.js Bento components are required to be R1. We have an equivalent static getPreconnects(element) in R1 designed to take care of this. Co-authored-by: Justin Ridgewell <justin@ridgewell.name> * ♻️ Add 'wrapper' designation to bento ampproject#34838 Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * 🚮 Delete init/constructor of `amp-soundcloud` No need to override `init` if no changes were made to it except calling parent class's `init`. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ Use `passthroughNonEmpty` which only give `<slot>` for non empty `children` Since `passthrough` gives a `<slot>` reference to the children prop in all cases, let's make this passthroughNonEmpty -- which will only give a `<slot>` if there are children in the given markup. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ Warning for required props when not present * ♻️ Warning for required pros when not present * ♻️ Warning for required props when not present * ♻️ `trackId` and `playlistId` order chek Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ `trackId` and `playlistId` order check Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ Removed explicitly specify boolean `true` for conditional block No need to explicitly specify boolean `true` for conditional block. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ Added default value for `visual` prop in Preact component Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * 🚮 Removed props handled by AMP's layout system `height`, `heights`, `media`, `sizes`, and `width` are not needed to pass through as these attributes are specific to AMP's layout system and handled by the runtime. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * 🚮 Removed unnecessary typedef `height`, `heights`, `media`, `sizes`, and `width` are not needed to pass through as these attributes are specific to AMP's layout system and handled by the runtime. Thus they do not need to be specify in typedef. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * 🚮 Removed interface as no API exposed * ♻️ Moved `converter.js` into storybook * 🚮 Removed `layout` from Preact example, ♻️ Added `height` into style * 🚮 Removed empty CSS and `hasCss` from `bundles.config.extensions.json` * 🐛 `htmlFor` import fix for test case, ♻️ added `data-trackid` attribute * ♻️ Replaced `<iframe>` with `<IframeEmbed>` * ♻️ Updated specification for validation * 🏗 Added `npm` to `bundles.config.extensions.json` Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com> * ♻️ New line at EOF in `validator-amp-soundcloud.out` * Added 'loadable' to indicate a resource intensive component * 🏗 Fixes reported by 'amp lint' for CI * 🏗 Fixes for aliased imports for CI Co-authored-by: Justin Ridgewell <justin@ridgewell.name> Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

AnuragVasanwala and others added 15 commits June 7, 2021 17:09

Initial Commit

1ddc3a3

Property mappings and Cleanup

c0bdf45

Minor fixes, Formatting and Comments

e8c6353

More readable variable name

4f0ced4

Test Cases for Bento

fe1920a

Updated test-cases and Storybook

6c4ab72

Merge branch 'ampproject:main' into bento/amp-soundcloud

b79254c

Added base CSS File

eb32b2f

Merge branch 'bento/amp-soundcloud' of https://github.com/rtCamp/amphtml

a2665f3

into bento/amp-soundcloud

Bug Fix: Layout System and Unit

773e788

Parent element changed from <div> to <>

f16e480

Type correction for 'visual' parameter

6d73ca4

Merge branch 'ampproject:main' into bento/amp-soundcloud

05ae49b

Bug fix for color parameter in storybook

62f5cef

Merge branch 'bento/amp-soundcloud' of https://github.com/rtCamp/amphtml

9fa0f1d

into bento/amp-soundcloud

amp-owners-bot bot requested a review from alanorozco June 11, 2021 10:35

kristoferbaxter requested a review from caroqliu June 11, 2021 18:31

jridgewell reviewed Jun 11, 2021

View reviewed changes

extensions/amp-soundcloud/1.0/base-element.js Outdated Show resolved Hide resolved

caroqliu reviewed Jun 11, 2021

View reviewed changes

extensions/amp-soundcloud/1.0/amp-soundcloud.css Show resolved Hide resolved

AnuragVasanwala and others added 8 commits June 14, 2021 07:19

Update extensions/amp-soundcloud/1.0/base-element.js

2b87c2e

Bento components are required to be R1. We have an equivalent static getPreconnects(element) in R1 designed to take care of this. Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

♻️ Add 'wrapper' designation to bento

901f11c

ampproject#34838 Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

🚮 Delete init/constructor of amp-soundcloud

3c05a23

No need to override `init` if no changes were made to it except calling parent class's `init`. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

♻️ Warning for required props when not present

7b45b0b

♻️ Warning for required pros when not present

cd0bdaa

♻️ Warning for required props when not present

512ddd7

Merge branch 'bento/amp-soundcloud' of https://github.com/rtCamp/amphtml

9b4c610

into bento/amp-soundcloud

AnuragVasanwala and others added 12 commits June 14, 2021 12:15

Merge branch 'bento/amp-soundcloud' of https://github.com/rtCamp/amphtml

47e93c0

into bento/amp-soundcloud

♻️ Removed explicitly specify boolean true for conditional block

4d96ee6

No need to explicitly specify boolean `true` for conditional block. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

♻️ Added default value for visual prop in Preact component

250d438

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

🚮 Removed interface as no API exposed

7750bc7

♻️ Moved converter.js into storybook

d1b78a3

🚮 Removed layout from Preact example, ♻️ Added height into style

51571a5

🚮 Removed empty CSS and hasCss from bundles.config.extensions.json

45f166c

🐛 htmlFor import fix for test case, ♻️ added data-trackid attribute

490f3ca

♻️ Replaced <iframe> with <IframeEmbed>

79ff0d1

♻️ Updated specification for validation

dc0eb86

caroqliu approved these changes Jun 15, 2021

View reviewed changes

build-system/compile/bundles.config.extensions.json Show resolved Hide resolved

extensions/amp-soundcloud/1.0/test/validator-amp-soundcloud.out Outdated Show resolved Hide resolved

AnuragVasanwala and others added 2 commits June 15, 2021 21:47

🏗 Added npm to bundles.config.extensions.json

dadc892

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

♻️ New line at EOF in validator-amp-soundcloud.out

a587d93

honeybadgerdontcare added the WG: caching label Jun 15, 2021

caroqliu reviewed Jun 17, 2021

View reviewed changes

extensions/amp-soundcloud/1.0/base-element.js Show resolved Hide resolved

Added 'loadable' to indicate a resource intensive component

86189e2

antiphoton approved these changes Jun 21, 2021

View reviewed changes

🏗 Fixes reported by 'amp lint' for CI

84b17a5

AnuragVasanwala and others added 2 commits June 23, 2021 21:18

Merge branch 'ampproject:main' into bento/amp-soundcloud

0951fd4

🏗 Fixes for aliased imports for CI

d4ac900

caroqliu merged commit 7d996a4 into ampproject:main Jun 24, 2021

	/** @override */
	pauseCallback() {
	const Ctor = this.constructor;
	if (Ctor['unloadOnPause']) {
	this.mutateProps(dict({'loading': Loading.UNLOAD}));
	this.resetLoading_ = true;
	} else {
	const {currentRef_: api} = this;
	api?.['pause']?.();
	}
	}

Conversation

AnuragVasanwala commented Jun 11, 2021

Uh oh!

CLAassistant commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amp-owners-bot bot commented Jun 11, 2021 • edited by ampprojectbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

caroqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caroqliu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

caroqliu Jun 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antiphoton left a comment

Choose a reason for hiding this comment

Uh oh!

caroqliu commented Jun 23, 2021

Uh oh!

caroqliu commented Jun 23, 2021

Uh oh!

AnuragVasanwala commented Jun 23, 2021

Uh oh!

AnuragVasanwala commented Jun 24, 2021

Uh oh!

caroqliu commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

CLAassistant commented Jun 11, 2021 •

edited

Loading

amp-owners-bot bot commented Jun 11, 2021 •

edited by ampprojectbot

Loading

caroqliu left a comment •

edited

Loading