✨ Require visibility trigger selector's to have data-vars-* #26902
Conversation
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
f2e6a66
to
d140559
Compare
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
a10426d
to
5179f8d
Compare
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
5179f8d
to
2dc62e8
Compare
|
@zhouyx PTAL |
| config = {}; | ||
|
|
||
| eventsSpy = env.sandbox.spy(tracker, 'onEvent_'); | ||
|
|
||
| target3 = win.document.createElement('div'); |
There was a problem hiding this comment.
I don't see the need to introduce the target3 here? Any reason we want a third element.
There was a problem hiding this comment.
I felt that it was important to test/show that array selector was using querySelectorAll here... but I guess we talked about this before and agreed that this functionality is handled by analytics root and so should be tested there...
Removing now
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
19480c6
to
9d15c94
Compare
|
@zhouyx Ready for review |
| @@ -300,7 +302,7 @@ export class AnalyticsRoot { | |||
| } | |||
| for (let j = 0; j < nodeList.length; j++) { | |||
| if (this.contains(nodeList[j])) { | |||
| elementArray.push(nodeList[j]); | |||
| this.checkElementDataVars(nodeList[j], elementArray, selector); | |||
There was a problem hiding this comment.
nit: Can we make this a private method.
Personally I would expect the checkElementDataVars to return a boolean based on the method name, and you won't need to pass the elementArray or selector. Not blocking.
There was a problem hiding this comment.
Changed the method.
| if (dataVarKeys.length) { | ||
| elementArray.push(element); | ||
| } else { | ||
| user().warn( |
There was a problem hiding this comment.
When multiple elements are selected. I think the message here will get duplicated.
One option is to also print the element information. A better option is to only print the warning message once with the number of elements that don't have data-vars- attribute specified. WDYT?
There was a problem hiding this comment.
Yes I agree, changed up the method to do slightly more work.
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
6db5518
to
1dc361a
Compare
micajuine-ho
force-pushed
the
restrict_to_data_vars
branch
from
1dc361a
to
0d7fac6
Compare
…ct#26902) * Restrict data-vars, impl, unit, manual test * Adding warning * Suggested changes
Task 3 of #26823
data-vars-*attribute withingetAmpElements()