-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
✨ [amp story captions] style-preset
attribute with styles
#37967
Conversation
0fb3e57
to
08d6642
Compare
data-preset
attribute with styles
There was a problem hiding this 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.
extensions/amp-story-captions/0.1/test/test-amp-story-captions.js
Outdated
Show resolved
Hide resolved
extensions/amp-story-captions/validator-amp-story-captions.protoascii
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
extensions/amp-story-captions/0.1/amp-story-captions-presets.css
Outdated
Show resolved
Hide resolved
data-preset
attribute with stylesstyle-preset
attribute with styles
|
||
.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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Warning: disparity between this PR Percy build and its 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 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 /
|
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
andfont-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:

Appear preset (customized):

Closes #37900