Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 36 commits into from Apr 18, 2022

Conversation

processprocess
Copy link
Contributor

@processprocess processprocess commented Mar 28, 2022

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
Copy link
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.

examples/amp-story/amp-story-captions.html Outdated Show resolved Hide resolved
extensions/amp-story-captions/0.1/amp-story-captions.js Outdated Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
extensions/amp-video/0.1/amp-video.js Outdated Show resolved Hide resolved
@banaag banaag self-requested a review April 14, 2022 18:52
Copy link
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.

@processprocess processprocess changed the title ✨ [amp story captions] data-preset attribute with styles ✨ [amp story captions] style-preset attribute with styles Apr 18, 2022

.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
Contributor

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
Contributor Author

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
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

[Amp story captions] Implement default, overridable CSS
5 participants