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

Add default values for AMP Timeago block #1586

Merged
merged 1 commit into from
Nov 4, 2018
Merged

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 2, 2018

Thanks to @csossi for reporting this in #1580.

Creating an AMP Timeago block with the default values causes a validation error in the Chrome extension because it doesn't have the required height value:

height-value-required

So this sets a default value of 20 for the height. This is the value in this example.

It also sets the default layout to fixed-height. This is the only layout available that doesn't require a width (there's no default width in the editor):

new-default-values

Fixes #1580

As Claudio reported,
there's an error when using the AMP Timeago block
with the default values
The layout is inially responsive,
which requires both a width an height.
And another option of fixed also requires a width and height.
So set fixed-height as the default layout,
as set a default height of 20.
This is the value in the documentation example.
So at least this block with the default values
won't have a validation error

@see https://www.ampproject.org/ko/docs/reference/components/amp-timeago
@kienstra
Copy link
Contributor Author

kienstra commented Nov 2, 2018

Request For Code Review

Hi @westonruter,
Could you please review this?

Thanks, and have a great weekend.

@kienstra kienstra requested a review from westonruter November 2, 2018 23:56
@westonruter westonruter added this to the v1.0 milestone Nov 4, 2018
@westonruter westonruter merged commit af51929 into 1.0 Nov 4, 2018
@westonruter westonruter deleted the fix/1580-amp-timeago branch November 4, 2018 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants