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

馃摉 Correcting a typo in uas.md file #18292

Merged
merged 2 commits into from Oct 2, 2018

Conversation

pm-harshad-mane
Copy link
Contributor

馃摉 Correcting a typo in uas.md file

  • Need to use accId instead of accountId.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@pm-harshad-mane
Copy link
Contributor Author

Hello @erwinmombay ,
We are again facing the same CLA related issue, can you help?

@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@@ -51,7 +51,7 @@ Note that the `width` and `height` mentioned should be maximum of the width-high
<amp-ad
width="300" height="250"
type="uas"
json='{"accId": "132109", "adUnit": "10002912", "sizes": [[300, 250]], "targetings": {"country": ["India", "USA"], "car": "Civic"}, "locLat": "12.24", "locLon": "24.13", "locSrc": "wifi", "pageURL": "http://mydomain.com"}'>
json='{"accId": "132109", "adUnit": "10002912", "sizes": [[300, 250]], "targetings": {"country": ["India", "USA"], "car": "Civic"}, "locLat": "12.24", "locLon": "24.13", "locSrc": "1", "pageURL": "http://mydomain.com"}'>
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't find the supported value for locSrc. Is 1 supported? wifi might actually make sense for "location source"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @lannka ,

Thank you for your suggestion.
PubMatic UAS tag needs to pass the value for locSrc parameter as per a pre-defined map of values thus we are changing it from "wifi" to "1".

Copy link
Contributor

@lannka lannka Sep 25, 2018

Choose a reason for hiding this comment

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

OK, that sounds good. But if you folks are the maintainer of PubMatic, I suggest you provide the full map in the documentation as well.

@pm-harshad-mane
Copy link
Contributor Author

Hello @lannka and @zhouyx ,
When can we merge these changes?

@lannka lannka merged commit 665b911 into ampproject:master Oct 2, 2018
@pm-harshad-mane
Copy link
Contributor Author

Thank you @lannka

torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Oct 10, 2018
* accId to be used instead of accountId

* updating value of locSrc param in documentation
Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
* accId to be used instead of accountId

* updating value of locSrc param in documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants