✨[Amp story captions] Use amp-story-captions for captions from cache by processprocess · Pull Request #38108 · ampproject/amphtml

processprocess · 2022-04-18T14:40:40Z

demo
** The demo is automatically appending from a track element since cached captions are not launched yet **

When appending captions from cache this PR

lazy loads the amp-story-captions component
appends it as a child to the video element with the default captions style
Appends a class to position the captions so they do not block cta UI.

Automatically appended captions are positioned so they do not block CTA UI. mockup:

mszylkowski · 2022-04-18T14:58:37Z

Can you append a screenshot of how these captions look? My worry is that the captions can be hidden if a video is on a background layer with an overlay (like on our pottery story), so we might want to add the captions onto a new top-level grid-layer so they are always visible on top. I remember this being one of the main complaints of the native captions

processprocess · 2022-04-18T15:01:14Z

Converting to draft. Blocked by #37967
We need to see how this looks before merging.

processprocess · 2022-04-18T15:02:21Z

Can you append a screenshot of how these captions look? My worry is that the captions can be hidden if a video is on a background layer with an overlay (like on our pottery story), so we might want to add the captions onto a new top-level grid-layer so they are always visible on top. I remember this being one of the main complaints of the native captions

Converting to draft until #37967 is merged.

gmajoulet · 2022-04-19T19:18:23Z

extensions/amp-story-captions/0.1/amp-story-captions.js

+    if (this.element.hasAttribute('auto-append')) {
+      this.container_.classList.add(`amp-story-captions-auto-append`);


I think you could set a class direclty from the JSX template

That wont work since this element is rendered in shadow dom.

The fact that style-preset=default behaves different depending on if we automatically appended it or not is tripping me a bit.

Can you provide an example of how you position the default preset as a publisher, and what the default position is if a publisher doesn't write any code?

It trips me a bit too.
The thought process is this:

If it's auto-appended we base it on the expected UX of captions and position it relative to the video.

It it's publisher specified the assumption is that it's intentionally placed.

The example for publisher writing no code is already in examples/amp-story/amp-story-captions.html as part of #37967

Okay it's a bit tricky, thank you for sharing.
Two things would be awesome if we could make them work together:

Using style-preset=default works awesome out of the box, without requiring a single line of CSS

Publishers can override the default position set by style-preset=default

If both things can't coexist, then let's keep things how you have them.

To follow up from our discussion:

It looks awesome out of the box both inline and auto-attached.

It can be placed anywhere when inline. When auto-attached it is locked in a position.

extensions/amp-story-captions/0.1/amp-story-captions-presets.css

extensions/amp-video/0.1/video-cache.js

extensions/amp-story-captions/0.1/amp-story-captions-presets.css

gmajoulet · 2022-04-20T17:17:08Z

extensions/amp-story-captions/0.1/amp-story-captions.js

+    if (this.element.hasAttribute('auto-append')) {
+      this.container_.classList.add(`amp-story-captions-auto-append`);


The fact that style-preset=default behaves different depending on if we automatically appended it or not is tripping me a bit.

Can you provide an example of how you position the default preset as a publisher, and what the default position is if a publisher doesn't write any code?

extensions/amp-video/0.1/amp-video.js

lgtm-com · 2022-04-21T15:46:20Z

This pull request introduces 1 alert when merging d11bf73 into 31c8027 - view on LGTM.com

new alerts:

1 for Superfluous trailing arguments

lgtm-com · 2022-04-21T19:23:00Z

This pull request introduces 1 alert when merging 0f15cbb into 01dad70 - view on LGTM.com

new alerts:

1 for Superfluous trailing arguments

lgtm-com · 2022-04-21T19:54:58Z

This pull request introduces 1 alert when merging 1eecd64 into 01dad70 - view on LGTM.com

new alerts:

1 for Superfluous trailing arguments

lgtm-com · 2022-04-22T14:28:58Z

This pull request introduces 1 alert when merging d2e8e12 into 01dad70 - view on LGTM.com

new alerts:

1 for Superfluous trailing arguments

ampprojectbot · 2022-04-25T14:22:55Z

Warning: disparity between this PR Percy build and its main build

The Percy build for this PR was approved (either manually by a member of the AMP team, or automatically if there were no visual diffs). However, during a continuous integration step we generated another Percy build using the commit on the main branch that this PR was merged into, and there appears to be a mismatch between the two.

This is possibly an indication of an issue with this pull request, but could also be the result of flakiness. Please inspect the two builds < This PR's Percy build / main commit's Percy build > and determine further action:

If the disparity appears to be caused by this PR, please create an bug report or send out a new PR to fix
If the disparity appears to be a flake, please @-mention ampproject/wg-approvers in a comment
If there is no disparity and this comment was created by mistake, please @-mention ampproject/wg-infra
If unsure, @-mention ampproject/wg-approvers

processprocess added 6 commits April 18, 2022 09:08

Lazy load captions. Add test.

9884fa3

Test for appending captions el. Helper for fetch.

d5e5d00

revert newline

ba148f8

Rename helper.

8903655

lint

e29a520

Update comment

e54bafb

processprocess added P1: High Priority WG: stories labels Apr 18, 2022

processprocess requested review from gmajoulet and mszylkowski April 18, 2022 14:40

processprocess self-assigned this Apr 18, 2022

processprocess mentioned this pull request Apr 18, 2022

[Amp story captions] Render video captions with amp-story-captions when in an amp-story doc and served from cache #37899

Closed

processprocess marked this pull request as draft April 18, 2022 15:00

processprocess added 3 commits April 19, 2022 09:04

Merge branch 'main' into install-captions

bdd4eec

auto-append class

76bdeeb

comment

4dd5f5c

processprocess marked this pull request as ready for review April 19, 2022 18:23

gmajoulet reviewed Apr 19, 2022

View reviewed changes

Install captions in amp-video so it's only loaded when needed.

a596707

processprocess requested a review from gmajoulet April 19, 2022 20:03

space

9db8544

gmajoulet reviewed Apr 20, 2022

View reviewed changes

Install after check. Add prefix to class.

e2d2fb8

processprocess marked this pull request as draft April 21, 2022 15:31

processprocess force-pushed the install-captions branch from d11bf73 to e2d2fb8 Compare April 21, 2022 18:39

ampdoc

9905b18

processprocess added 2 commits April 21, 2022 13:42

spelling

17c2a06

arg

683f0b0

processprocess marked this pull request as ready for review April 21, 2022 18:59

param

0f15cbb

z index

1eecd64

Merge branch 'main' into install-captions

d2e8e12

arg

5848303

gmajoulet approved these changes Apr 22, 2022

View reviewed changes

processprocess merged commit 4db72a2 into ampproject:main Apr 25, 2022

processprocess deleted the install-captions branch April 25, 2022 14:05

mszylkowski approved these changes Apr 25, 2022

View reviewed changes

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels May 5, 2022

ampprojectbot added the PR use: In LTS label Jun 13, 2022

		if (this.element.hasAttribute('auto-append')) {
		this.container_.classList.add(`amp-story-captions-auto-append`);

Conversation

processprocess commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mszylkowski commented Apr 18, 2022

Uh oh!

processprocess commented Apr 18, 2022

Uh oh!

processprocess commented Apr 18, 2022

Uh oh!

gmajoulet Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

processprocess Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

gmajoulet Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

processprocess Apr 20, 2022 • 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!

gmajoulet Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

processprocess Apr 22, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmajoulet Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lgtm-com bot commented Apr 21, 2022

Uh oh!

lgtm-com bot commented Apr 21, 2022

Uh oh!

lgtm-com bot commented Apr 21, 2022

Uh oh!

lgtm-com bot commented Apr 22, 2022

Uh oh!

ampprojectbot commented Apr 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

processprocess commented Apr 18, 2022 •

edited

Loading

processprocess Apr 20, 2022 •

edited

Loading