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
Fix layouts and host for GMOSSP amp-ads #10566
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. 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, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
Could @keithwrightbos review my pull request? |
Hi @jridgewell, @keithwrightbos, Could you review my pull request? Please make sure everything is done smoothly. Best regards, |
const GMOSSP_BASE_URL_ = 'https://sp.gmossp-sp.jp'; | ||
const GMOSSP_BASE_URL_ = 'https://sp.gmossp-sp.jp/'; | ||
|
||
const GMOSSP_BASE_A4A_URL_ = 'https://amp.sp.gmossp-sp.jp/_a4a/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typedoc?
@@ -49,7 +51,8 @@ export class AmpAdNetworkGmosspImpl extends AmpA4A { | |||
|
|||
/** @override */ | |||
getAdUrl() { | |||
return this.element.getAttribute('src'); | |||
return this.element.getAttribute('src').replace(GMOSSP_BASE_URL_, | |||
GMOSSP_BASE_A4A_URL_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that isValidElement expects the old url format. Do you want to update to also allow a4a path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you need to modify isValidElement to allow either GMOSSP_BASE_A4A_URL_ or GMOSSP_BASE_URL_. You will also need to modify extensions/amp-ad-network-gmossp-impl/0.1/gmossp-a4a-config#gmosspIsA4AEnabled
@keithwrightbos |
@@ -49,7 +51,8 @@ export class AmpAdNetworkGmosspImpl extends AmpA4A { | |||
|
|||
/** @override */ | |||
getAdUrl() { | |||
return this.element.getAttribute('src'); | |||
return this.element.getAttribute('src').replace(GMOSSP_BASE_URL_, | |||
GMOSSP_BASE_A4A_URL_); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you need to modify isValidElement to allow either GMOSSP_BASE_A4A_URL_ or GMOSSP_BASE_URL_. You will also need to modify extensions/amp-ad-network-gmossp-impl/0.1/gmossp-a4a-config#gmosspIsA4AEnabled
@keithwrightbos |
Hi @jridgewell, @keithwrightbos, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for the delay
Could you merge 'Fix layouts and host for GMOSSP amp-ads' into ampproject/amphtml master branch?
We have a Google Corporate CLA.
Our CLA: GMO AD Marketing Inc.
Our company name: GMO AD Marketing Inc.
Thank you in advance