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

🐛Update validator-amp-base-carousel.protoascii with 'controls' attribute. Resolves 31872 #31873

Merged
merged 2 commits into from
Jan 26, 2021

Conversation

Notebit
Copy link
Contributor

@Notebit Notebit commented Jan 9, 2021

Missing "controls" attribute, which is can take a media query to resolve to its value as one of "auto"|"always"|"never".
https://amp.dev/documentation/components/amp-base-carousel/?format=websites#controls

Example valid value in media query format:
(min-width: 1000px) always, (min-width: 600px) never, always

Resolves #31872

@CLAassistant
Copy link

CLAassistant commented Jan 9, 2021

CLA assistant check
All committers have signed the CLA.

@amp-owners-bot
Copy link

amp-owners-bot bot commented Jan 9, 2021

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

extensions/amp-base-carousel/validator-amp-base-carousel.protoascii

@honeybadgerdontcare
Copy link
Contributor

@ampproject/wg-components can you confirm that this is what the WG intended?

@honeybadgerdontcare
Copy link
Contributor

@Notebit the reason I'm asking components is because attr-lists explicitly include attrs and components may have intentionally structured it this way.

@Notebit
Copy link
Contributor Author

Notebit commented Jan 9, 2021

@honeybadgerdontcare I am a complete newbie, so please feel free to do any check you think is necessary. any help is appreciated.

Copy link
Contributor

@caroqliu caroqliu left a comment

Choose a reason for hiding this comment

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

Made one suggestion to use regex to support media query style values, copied from the style employed in the rest of the file. Also pinging @micajuine-ho to review.

@twifkak twifkak self-requested a review January 11, 2021 23:21
@caroqliu
Copy link
Contributor

ping @ampproject/wg-caching

@honeybadgerdontcare
Copy link
Contributor

@caroqliu what is the ping for?

@honeybadgerdontcare
Copy link
Contributor

@Gregable for validator file review

@caroqliu caroqliu merged commit 030d76b into ampproject:master Jan 26, 2021
@Notebit
Copy link
Contributor Author

Notebit commented Jan 26, 2021

thanks everybody for the prompt support

@Notebit Notebit deleted the patch-1 branch January 26, 2021 21:07
PetrBlaha pushed a commit to PetrBlaha/amphtml that referenced this pull request Jan 28, 2021
…ute. Resolves 31872 (ampproject#31873)

* Update validator-amp-base-carousel.protoascii

Missing "controls" attribute
https://amp.dev/documentation/components/amp-base-carousel/?format=websites#controls

* Support media query syntax

https://amp.dev/documentation/components/amp-base-carousel/#media-queries

Co-authored-by: Caroline Liu <10456171+caroqliu@users.noreply.github.com>
honeybadgerdontcare pushed a commit to honeybadgerdontcare/amphtml that referenced this pull request Feb 2, 2021
honeybadgerdontcare added a commit that referenced this pull request Feb 2, 2021
* cl/354149380 Revision bump for #31873

* cl/354558578 Deprecate `lang` attribute on `doctype`.

* cl/354593816 Revision bump for #32262

* revert duplicate controls

* cl/354600080 Revision bump for #32288

* cl/355043682 n/a

Co-authored-by: Greg Grothaus <greggrothaus@google.com>
Co-authored-by: Amaltas Bohra <amaltas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[amp-base-carousel] The attribute 'controls' may not appear in tag 'amp-base-carousel'.
7 participants