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.

Sign up for GitHub

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

Jump to bottom

Validate amp-base-carousel and friends to enable loop on empty value #35568

Merged
merged 6 commits into from
Aug 11, 2021
Merged

Validate amp-base-carousel and friends to enable loop on empty value #35568

merged 6 commits into from
Aug 11, 2021

Conversation

caroqliu
Copy link
Contributor

@caroqliu caroqliu commented Aug 9, 2021 •
edited

For all components, this PR validates loop attribute with no value. Feature-wise, using loop with no attribute should be logically equivalent to enabling the feature. For 1.0 this involves no changes, but for 0.1 this involves updating the default behavior to allow for empty strings. This should not be a breaking change given that, prior to this PR, using the attribute without a value would be invalid AMP.

Fixes #35555

@amp-owners-bot
Copy link

amp-owners-bot bot commented Aug 9, 2021 •
edited by ampprojectbot

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

extensions/amp-base-carousel/0.1/test/validator-amp-base-carousel.html
extensions/amp-base-carousel/0.1/test/validator-amp-base-carousel.out
extensions/amp-base-carousel/1.0/test/validator-amp-base-carousel.html
extensions/amp-base-carousel/1.0/test/validator-amp-base-carousel.out
extensions/amp-base-carousel/validator-amp-base-carousel.protoascii
extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-thumbnails.html
extensions/amp-inline-gallery/0.1/test/validator-amp-inline-gallery-thumbnails.out
extensions/amp-inline-gallery/1.0/test/validator-amp-inline-gallery-thumbnails.html
extensions/amp-inline-gallery/1.0/test/validator-amp-inline-gallery-thumbnails.out
extensions/amp-inline-gallery/validator-amp-inline-gallery.protoascii
extensions/amp-stream-gallery/1.0/test/validator-amp-stream-gallery.html
extensions/amp-stream-gallery/1.0/test/validator-amp-stream-gallery.out
+1 more

honeybadgerdontcare
honeybadgerdontcare approved these changes Aug 9, 2021
View reviewed changes
Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

approved for validation related changes

extensions/amp-stream-gallery/1.0/test/validator-amp-stream-gallery.html Outdated
@@ -32,7 +32,6 @@
<section>
<!-- Valid: true -->
<amp-stream-gallery width="4" height="3" loop="true">
</amp-stream-gallery>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this removal intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. fixed, thanks!

@caroqliu caroqliu merged commit 47c5394 into ampproject:main Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmanek dmanek dmanek approved these changes

@honeybadgerdontcare honeybadgerdontcare honeybadgerdontcare approved these changes

@alanorozco alanorozco Awaiting requested review from alanorozco

Assignees
No one assigned
Labels
PR use: In Beta / Experimental PR use: In LTS PR use: In Stable
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot use loop as boolean attribute for amp-base-carousel
4 participants
@caroqliu @dmanek @honeybadgerdontcare @ampprojectbot