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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix story ads in no signing exp #30224

Merged
merged 3 commits into from Sep 14, 2020
Merged

馃悰 Fix story ads in no signing exp #30224

merged 3 commits into from Sep 14, 2020

Conversation

calebcordry
Copy link
Member

In the normal case the cta url & cta type are extracted from the metadata obj and added as data-var attrs to the element. In the no-signing case the metadata obj does not exist. This PR extends what we query for in the ad doc to find the <meta name=amp-cta-*> elements.

Should fix broken experimentA on master builds.

Copy link
Contributor

@lannka lannka left a comment

Choose a reason for hiding this comment

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

could you remind me why we had these 2 formats of prefix?

* @return {!IArrayLike}
*/
export function getAmpCtaMetaTags(doc) {
return doc.querySelectorAll('meta[name^=amp-cta-]');
Copy link
Contributor

Choose a reason for hiding this comment

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

did you try one query to select both?
meta[name^=amp-cta-],meta[name^=amp4ads-vars-]

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to single selector.

@calebcordry
Copy link
Member Author

We started with amp-cta-url & amp-cta-type but later we thought it might be good to give all these the same amp4ads-vars prefix so that we could potentially introduce these to non-story case if needed.

@calebcordry calebcordry merged commit e28f587 into ampproject:master Sep 14, 2020
@calebcordry calebcordry deleted the fix-story-no-sign branch September 14, 2020 21:39
ed-bird pushed a commit to ed-bird/amphtml that referenced this pull request Dec 10, 2020
* look for more tags

* use single selector

* cleanup
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.

None yet

3 participants