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

Videonow amp-ad debug ability #25217

Merged
merged 10 commits into from Oct 31, 2019
Merged

Videonow amp-ad debug ability #25217

merged 10 commits into from Oct 31, 2019

Conversation

sdbaron
Copy link
Contributor

@sdbaron sdbaron commented Oct 23, 2019

♻️ removed old code
✅ added ability to set vn_debug, vn_module, vn_init_module in the location url

Copy link
Contributor

@powerivq powerivq left a comment

Choose a reason for hiding this comment

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

Just recommendations. Ping me after you respond.

ads/videonow.js Outdated
let customTag = null;
const gn = global && global.name;
if (gn) {
const p = JSON.parse(gn);
Copy link
Contributor

Choose a reason for hiding this comment

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

Would recommend

if (global && global.name) {
    const p = JSON.parse(global.name);

Little bit of twisting needed to follow the logic here, though I think they are equivalent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ок

ads/videonow.js Outdated Show resolved Hide resolved
ads/videonow.js Outdated
p.attributes._context.location &&
p.attributes._context.location.href;

if (href) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, could do if (p && ...) { here and then declare href.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

ads/videonow.js Outdated Show resolved Hide resolved
ads/videonow.js Outdated
if (href) {
const vnDataStorageKey = 'videonow-config';
const logLevelParsed = /[?&]vn_debug\b(?:=(\d+))?/.exec(href);
const vnModuleParsed = /[?&]vn_module=(.*?)($|&)/.exec(href);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could do vn_module=[^&]* so it always stops at the next & and so it doesn't depend on the greediness of the search.

@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 24, 2019

@powerivq
Found forbidden: "localStorage" in ads/videonow.js:48:39
Can I somehow get permission to use localStorage and sessionStorage?

@powerivq
Copy link
Contributor

@sdbaron I was wondering that. Can you explain your use case here? Why is logging to console not sufficient for debugging? There is a very high bar for using localstorage/sessionstorage for privacy concerns.

@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 25, 2019

@sdbaron I was wondering that. Can you explain your use case here? Why is logging to console not sufficient for debugging? There is a very high bar for using localstorage/sessionstorage for privacy concerns.

It’s too difficult to explain all the processes inside our advertising module. I changed a couple lines of code and now there is no need to use sessionStorage.

@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 25, 2019

@powerivq Should I do something with this Missing required OWNERS approvals?

@powerivq
Copy link
Contributor

@sdbaron nope. It'll pass once the PR is approved.

@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 30, 2019

@powerivq Hi! Should I do something else with this PR?

@powerivq
Copy link
Contributor

Do you want me to merge? @sdbaron

@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 31, 2019

Do you want me to merge? @sdbaron

Yes please. I am not authorized to merge PR.

@powerivq powerivq merged commit 3eed955 into ampproject:master Oct 31, 2019
@sdbaron
Copy link
Contributor Author

sdbaron commented Oct 31, 2019

@powerivq thanks a lot

@sdbaron sdbaron deleted the vn.url.parameters branch October 31, 2019 22:05
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
* -- change default URL of vn_init_module
-- forwards parameters vn_debug and vn_module

* -- add ability to set vn_init_module in the location url
-- changed demo profileId to 5555

* -- some code style fixed

* -- stop using sessionStore and localStorage

* -- fixed code style in videonow.md

* -- changed JSON.parse to parseJson

* -- changed some code for pr rules
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

3 participants