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 amp-ad-exit docs #11550

Merged
merged 2 commits into from Dec 14, 2017
Merged

Update amp-ad-exit docs #11550

merged 2 commits into from Dec 14, 2017

Conversation

clawr
Copy link
Contributor

@clawr clawr commented Oct 3, 2017

to reflect changes in #11462

to reflect changes in #11462
Copy link
Contributor

@jonkeller jonkeller left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

@@ -273,8 +276,7 @@ Example:
},
"_3pAnalytics": {
"defaultValue": "no_response",
"vendorAnalyticsSource": "VendorXYZ",
"vendorAnalyticsResponseKey": "findings"
"iframeTransportSignal": "IFRAME_TRANSPORT_SIGNAL(example-3p-vendor,collected-data)"
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM. You might add a note that (at least for now) adding a space after this comma is not allowed. (See #11461 (comment) )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done

Copy link
Contributor

Choose a reason for hiding this comment

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

(Note that the comma thing is no longer true...)

Copy link
Contributor

Choose a reason for hiding this comment

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

@calebcordry is working on nested function calls in URL replacement. we might again restrict the space use here.

Copy link
Member

Choose a reason for hiding this comment

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

I am not 100% yet. There might be a way to preserve spacing after comma only. I am still thinking it through.

@clawr clawr requested a review from lannka October 9, 2017 18:58
@ampprojectbot ampprojectbot added this to the Pending Triage milestone Oct 9, 2017
@lannka
Copy link
Contributor

lannka commented Dec 7, 2017

@clawr shall we merge this?

@lannka lannka merged commit e8ce880 into master Dec 14, 2017
@lannka lannka deleted the clawr-patch-1 branch December 14, 2017 22:34
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
* Update amp-ad-exit docs

to reflect changes in ampproject#11462

* Update amp-ad-exit.md
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

6 participants