✨ Bento `amp-audio` by AnuragVasanwala · Pull Request #35342 · ampproject/amphtml

AnuragVasanwala · 2021-07-21T20:40:57Z

The goal of the PR is to add a Bento amp-audio extension. Although component is working but there are few issues which needs to be resolved.

Checklist

Issues (Resolved)

1. Failing test cases:

2. In Bento AMP context, due to shadow DOM, `<source .../>` child are not enumerating into `<audio ... />` component, instead `<slot ... />` is enumerated:

Preact component with loading through sources works ✅:

<Audio
     height="75"
     width="auto"
     controls
   >
     <source
       src="https://storage.googleapis.com/media-session/sintel/snow-fight.mp3"
       type="audio/mpeg"
     />
</Audio>

Bento AMP does not work loading through sources ❌:

<amp-audio
      height="75"
      width="auto"
      layout="fixed-height"
      controls
    >
      <source
        src="https://storage.googleapis.com/media-session/sintel/snow-fight.mp3"
        type="audio/mpeg"
      />
      <div fallback>Your browser doesn’t support HTML5 audio</div>
</amp-audio>

Please provide necessary feedback 💡

lgtm-com · 2021-07-21T21:14:43Z

This pull request introduces 1 alert when merging 3436362 into 2e38479 - view on LGTM.com

new alerts:

1 for Useless conditional

caroqliu

Thanks for working on this! Sending first pass now helps get sources working in the AMP layer with some additional recommendations. I'll take a look at tests momentarily.

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

extensions/amp-audio/1.0/amp-audio.js

extensions/amp-audio/1.0/component.js

extensions/amp-audio/1.0/component.type.js

caroqliu

Testing comments added ⬇️

extensions/amp-audio/1.0/test/test-amp-audio.js

extensions/amp-audio/1.0/component.js

extensions/amp-audio/1.0/test/test-amp-audio.js

…xtensions.json`

… into add/amp-audio-bento

`toggleFallback` can be on `onLoad` / `onError` / etc. instead. The Preact layer does not care what logic is passed here, only when it should be called. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

Added support to load audio from multiple `<source>` tags. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

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

`useRef` should be initialised with `null`. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

honeybadgerdontcare

minor nit and a question regarding lock files, otherwise lgtm for validation

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

validator/package-lock.json

validator/js/nodejs/package-lock.json

Running `amp validator --update_tests` have updated `package-lock.json` files. Co-Authored-By: honeybadgerdontcare <1340565+honeybadgerdontcare@users.noreply.github.com>

AnuragVasanwala · 2022-04-06T19:18:43Z

Hi @alanorozco & @jridgewell 👋🏻

Would you please review this PR and provide necessary feedback.

extensions/amp-audio/1.0/component.js

…rge conflicts, if any.

* cl/441530960 Two-way sync for PR #35342. No-op, or fixes merge conflicts, if any. * Fix incorrect merge Co-authored-by: Devin Mullins <twifkak@google.com>

ediamin and others added 13 commits June 24, 2021 15:13

Initiate amp-audio bento component

cec7c79

Make Audio component functional

89f9d5a

Merge branch 'ampproject:main' into add/amp-audio-bento

612f499

♻️ Initial Test Component

db26be9

Merge branch 'ampproject:main' into add/amp-audio-bento

dc2c46b

Merge branch 'ampproject:main' into add/amp-audio-bento

efb754d

Merge branch 'ampproject:main' into add/amp-audio-bento

f4fc2b9

🧪 Experimental Test Case Implementation

8265ce8

🧪 Refactoring

953cc6a

Merge branch 'ampproject:main' into add/amp-audio-bento

3000edb

♻️ Comments and Prettify

62332a5

🐛 Minor fixes

ef55355

♻️ Minor fix for test case

3436362

♻️ propagateAttributes moved to amp-audio.js

625ac36

caroqliu self-requested a review July 26, 2021 15:35

caroqliu requested changes Jul 26, 2021

View reviewed changes

caroqliu reviewed Jul 27, 2021

View reviewed changes

AnuragVasanwala and others added 12 commits July 28, 2021 12:52

Merge branch 'ampproject:main' into add/amp-audio-bento

3490ac7

🚮 Removed duplicate entry for amp-date-display in `bundles.config.e…

9b3a7fe

…xtensions.json`

Merge branch 'add/amp-audio-bento' of https://github.com/rtCamp/amphtml…

44ba6bb

… into add/amp-audio-bento

♻️ isStoryDescendent is moved to isolated AMP layer

494eb45

♻️ toggleFallback refactored into onLoad and onError

3c325c2

`toggleFallback` can be on `onLoad` / `onError` / etc. instead. The Preact layer does not care what logic is passed here, only when it should be called. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

🚮 Removed unnecessary toggleFallback from component.js

15a4dcf

🚮 Removed unnecessary comments

1edca8f

🐛 Fixed support to load from <source>

4bc85fe

Added support to load audio from multiple `<source>` tags. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

🚮 Removed unnecessary commented code-block

720ef01

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

♻️ Initial value updated for audioRef

5f4fd5b

`useRef` should be initialised with `null`. Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>

Merge branch 'ampproject:main' into add/amp-audio-bento

6053707

♻️ props destructured into inline

d060ad4

AnuragVasanwala requested a review from honeybadgerdontcare August 30, 2021 13:27

honeybadgerdontcare approved these changes Aug 30, 2021

View reviewed changes

extensions/amp-audio/1.0/test/validator-amp-audio.html Outdated Show resolved Hide resolved

validator/package-lock.json Show resolved Hide resolved

validator/js/nodejs/package-lock.json Show resolved Hide resolved

🏗 Updated validation rules

1feb141

Running `amp validator --update_tests` have updated `package-lock.json` files. Co-Authored-By: honeybadgerdontcare <1340565+honeybadgerdontcare@users.noreply.github.com>

westonruter added the WG: bento label Sep 5, 2021

AnuragVasanwala and others added 7 commits April 4, 2022 23:20

Merge branch 'ampproject:main' into add/amp-audio-bento

10713e3

♻️ Fix import alias

93c70fa

♻️ Update derived class extends using setSuperClass

d9b3d89

Fix test case and build issue

d2a1851

Remove addon-knobs dependency from storybook files

23e9a85

Update bundles config with new properties

73fb052

Merge branch 'ampproject:main' into add/amp-audio-bento

54dc21f

jridgewell approved these changes Apr 7, 2022

View reviewed changes

extensions/amp-audio/1.0/component.js Show resolved Hide resolved

AnuragVasanwala added 2 commits April 8, 2022 12:16

Merge branch 'ampproject:main' into add/amp-audio-bento

10dbeb2

Merge branch 'ampproject:main' into add/amp-audio-bento

40506f6

dethstrobe approved these changes Apr 12, 2022

View reviewed changes

dethstrobe enabled auto-merge (squash) April 12, 2022 15:18

🖍 Add CSS (as per ampproject#32180)

41fa12d

auto-merge was automatically disabled April 12, 2022 15:47
Head branch was pushed to by a user without write access

dethstrobe enabled auto-merge (squash) April 12, 2022 16:09

♻️ Prefix Bento to Preact Component (BentoAudio)

6c85ef8

auto-merge was automatically disabled April 12, 2022 16:15
Head branch was pushed to by a user without write access

alanorozco approved these changes Apr 12, 2022

View reviewed changes

dethstrobe enabled auto-merge (squash) April 12, 2022 16:27

dethstrobe merged commit 8ae5d1a into ampproject:main Apr 12, 2022

banaag pushed a commit to banaag/amphtml that referenced this pull request Apr 19, 2022

cl/441530960 Two-way sync for PR ampproject#35342. No-op, or fixes me…

2a84143

…rge conflicts, if any.

banaag mentioned this pull request Apr 19, 2022

Validator Rollup #38115

Merged

banaag added a commit that referenced this pull request Apr 19, 2022

Validator Rollup (#38115)

79b5c24

* cl/441530960 Two-way sync for PR #35342. No-op, or fixes merge conflicts, if any. * Fix incorrect merge Co-authored-by: Devin Mullins <twifkak@google.com>

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Apr 21, 2022

Conversation

AnuragVasanwala commented Jul 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Issues (Resolved)

1. Failing test cases:

2. In Bento AMP context, due to shadow DOM, <source .../> child are not enumerating into <audio ... /> component, instead <slot ... /> is enumerated:

Uh oh!

lgtm-com bot commented Jul 21, 2021

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!

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!

honeybadgerdontcare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnuragVasanwala commented Apr 6, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

AnuragVasanwala commented Jul 21, 2021 •

edited

Loading

2. In Bento AMP context, due to shadow DOM, `<source .../>` child are not enumerating into `<audio ... />` component, instead `<slot ... />` is enumerated: