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

Allow amp-selector in AMP4ADS #25773

Closed
jyn15 opened this issue Nov 26, 2019 · 8 comments · Fixed by #25889 or #26025
Closed

Allow amp-selector in AMP4ADS #25773

jyn15 opened this issue Nov 26, 2019 · 8 comments · Fixed by #25889 or #26025

Comments

@jyn15
Copy link
Contributor

jyn15 commented Nov 26, 2019

amp-form is allowed in amp4ads, and the doc says amp-selector is allowed in amp-form:
https://amp.dev/documentation/components/amp-form/?format=ads#inputs-and-fields

Is there any reason why amp-selector itself is not allowed in amp4ads?

@lannka
Copy link
Contributor

lannka commented Nov 26, 2019

seems reasonable.
@ampproject/wg-ads @ampproject/wg-runtime @ampproject/wg-ui-and-a11y any objections?

@jyn15
Copy link
Contributor Author

jyn15 commented Dec 4, 2019

btw, this allows us achieving more customized carousel buttons in amp-carousel-0.2
https://amp-playground.appspot.com/amp/5639955095224320

@lannka
Copy link
Contributor

lannka commented Dec 4, 2019

@jyn15 you can go ahead and make the validator change. you will be updating:
https://github.com/ampproject/amphtml/blob/master/extensions/amp-selector/validator-amp-selector.protoascii

/cc @honeybadgerdontcare correct me

@honeybadgerdontcare
Copy link
Contributor

@jyn15 you can go ahead and make the validator change. you will be updating:
https://github.com/ampproject/amphtml/blob/master/extensions/amp-selector/validator-amp-selector.protoascii

/cc @honeybadgerdontcare correct me

That is correct. Any tag spec that has html_format: AMP, add underneath it html_format: AMP4ADS. Probably should also add a specific test file in amp4ads_feature_tests as well.

@jyn15
Copy link
Contributor Author

jyn15 commented Dec 4, 2019

Thanks! I will make the change.

@lannka
Copy link
Contributor

lannka commented Dec 6, 2019

@jyn15 could you please also update amp-a4a-format.md file

@jyn15
Copy link
Contributor Author

jyn15 commented Dec 14, 2019

Btw, do you know how to remove the banner on the top of page
https://amp.dev/documentation/components/amp-selector/?format=ads
saying "this component does not support your currently selected format ads!"

@honeybadgerdontcare
Copy link
Contributor

@CrystalOnScript @pbakaus regarding the banner message

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