Skip to content

✨ [amp story captions] style-preset attribute with styles#37967

Merged
processprocess merged 36 commits intoampproject:mainfrom
processprocess:captions-style
Apr 18, 2022
Merged

✨ [amp story captions] style-preset attribute with styles#37967
processprocess merged 36 commits intoampproject:mainfrom
processprocess:captions-style

Conversation

@processprocess
Copy link
Copy Markdown
Contributor

@processprocess processprocess commented Mar 28, 2022 •
edited
Loading

Allows an optional style-preset attribute that when defined and valid renders captions in shadow dom and adds custom styles.
Adds tests.
Updates validation.
Implements the "default" and "appear" themes.

Appear theme has overridable font-family and font-size values using css variables.
CSS variables are used so default values can be defined and overridden on the presets.

note: docs will need to be updated with how to set up and customize the default presets.

Demo pages 2 is "default" theme, page 3 is "appear" theme.

Default preset:
Screen Shot 2022-04-27 at 9 56 43 AM

Appear preset (customized):
Screen Shot 2022-04-27 at 9 57 35 AM

Closes #37900

@processprocess processprocess changed the title 🖍 [amp story captions] Default overridable CSS 🖍 [amp story captions] data-preset attribute with styles Apr 13, 2022
@processprocess processprocess marked this pull request as ready for review April 13, 2022 16:21
@amp-owners-bot
Copy link
Copy Markdown

amp-owners-bot bot commented Apr 13, 2022 •
edited by ampprojectbot
Loading

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

extensions/amp-story-captions/0.1/test/validator-amp-story-captions.html
extensions/amp-story-captions/0.1/test/validator-amp-story-captions.out
extensions/amp-story-captions/validator-amp-story-captions.protoascii

mszylkowski
mszylkowski reviewed Apr 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mszylkowski mszylkowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropped some questions on the functionality and implementation.

gmajoulet
gmajoulet reviewed Apr 14, 2022
View reviewed changes
@banaag banaag self-requested a review April 14, 2022 18:52
banaag
banaag approved these changes Apr 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@banaag banaag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validator changes LGTM after gmajoulet's comment is fixed.

mszylkowski
mszylkowski reviewed Apr 14, 2022
View reviewed changes
@processprocess processprocess changed the title ✨ [amp story captions] data-preset attribute with styles ✨ [amp story captions] style-preset attribute with styles Apr 18, 2022
@processprocess processprocess mentioned this pull request Apr 18, 2022
Merged
gmajoulet
gmajoulet approved these changes Apr 18, 2022
View reviewed changes
extensions/amp-story-captions/0.1/amp-story-captions-presets.css

.amp-story-captions-default .amp-story-captions-cue-wrapper {
font-size: calc(2.5 * var(--story-page-vh, 8px)) !important;
font-family: 'Roboto', sans-serif !important;
Copy link
Copy Markdown
Contributor

@gmajoulet gmajoulet Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this use Helvetica on iOS?

Copy link
Copy Markdown
Contributor Author

@processprocess processprocess Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On MacOs Yes

Screen Shot 2022-04-18 at 11 42 39 AM

It may use San Fransisco on iOS, which is the system font. Double checking.

@processprocess processprocess merged commit c813c13 into ampproject:main Apr 18, 2022
@processprocess processprocess deleted the captions-style branch April 18, 2022 16:48
@ampprojectbot
Copy link
Copy Markdown
Member

ampprojectbot commented Apr 18, 2022

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 processprocess mentioned this pull request Apr 25, 2022
Open
@jshamble jshamble mentioned this pull request May 20, 2022
Open
@swissspidy swissspidy mentioned this pull request May 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmajoulet gmajoulet gmajoulet approved these changes

@banaag banaag banaag approved these changes

@mszylkowski mszylkowski mszylkowski approved these changes

@jshamble jshamble Awaiting requested review from jshamble

Assignees

No one assigned

Labels

PR use: In Beta / Experimental PR use: In Stable WG: caching

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Amp story captions] Implement default, overridable CSS

5 participants

@processprocess @ampprojectbot @gmajoulet @banaag @mszylkowski