-
Notifications
You must be signed in to change notification settings - Fork 177
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
Monetization: Add MGID support #13368
Conversation
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.
Sweet, thank you! I was not aware of this new support. At first glance this all looks good. Just a few minor comments.
234b3ee
to
b56ea36
Compare
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.
Getting there! My suggestions should help fix the tests. Just a few formatting issues in in addition to that. Try running Prettier with npm run lint:js:fix
packages/wp-dashboard/src/components/editorSettings/test/editorSettings.js
Outdated
Show resolved
Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/mgid/index.js
Outdated
Show resolved
Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/test/adManagement.js
Outdated
Show resolved
Hide resolved
Tried my best to fix all linter and unit tests errors. Let's run pipeline checks again. |
Looks like all the relevant tests are passing now 🎉 The remaining ones are known to be flakey, so can be ignored. |
801a5f4
to
cb3133c
Compare
@swissspidy Can you re-check latest update? |
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.
Minor nitpick, otherwise LGTM!
packages/wp-dashboard/src/components/editorSettings/adManagement/index.js
Show resolved
Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/mgid/index.js
Show resolved
Hide resolved
@swissspidy @AnuragVasanwala can we come back to string? Now all changes from string to number produce many issues and workarounds to work with default null (undefined in JS) value. |
Let me take a look 👍 |
@leprik Absolutely, if it is causing issues, please revert back to the |
@leprik I have only acknowledged for the changes highlighted by my review: Let's wait for the @swissspidy review on the changes highlighted by him earlier related to the latest commit 72df86f. |
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.
As long as a string works fine with MGID to display ads, that sounds good to me :-)
Context
MGID is now support Web Story ads and we want to add to WP plugin.
https://amp.dev/documentation/guides-and-tutorials/develop/advertise_amp_stories
Summary
MGID integration
Relevant Technical Choices
Source code is similar to Ad Manager
To-do
Not applicable.
User-facing changes
Testing Instructions
(Other formats should cause warnings / red borders)
Reviews
Does this PR have a security-related impact?
No.
Does this PR change what data or activity we track or use?
No.
Does this PR have a legal-related impact?
No.
Checklist
Type: XYZ
label to the PR