Added code to validate visibilitySpec in amp-analytics. #2743
Conversation
|
@avimehta Is this ready to be reviewed? |
avimehta
force-pushed
the
ads
branch
2 times, most recently
from
b7e2cad to
dfe7f8f
Compare
|
@dvoytenko ready for review. |
|
/to @cramforce for review |
| // TODO(avimehta, #1297): Add all the listeners so that visibility | ||
| // conditions are monitored and callback is called when the conditions | ||
| // are met. | ||
| if (this.viewer_.isVisible()) { |
There was a problem hiding this comment.
These two code paths aren't quite similar. The consequent will only call callback once, the alternate will call it every time the visibility changes to visible. Is that intended?
There was a problem hiding this comment.
Thanks for pointing out. Since this PR is about visibilitySpec, I'll not touch the bug(unless you insist). I'll send out another PR with the fix for this bug.
There was a problem hiding this comment.
Oh, I see this was moved. Another PR would be perfect.
|
ptal. |
| const ttMin = spec['totalTimeMin']; | ||
|
|
||
| // "", 1, 0, undefined, 100, 101 are positive. -1, NaN are not. | ||
| function isPositiveNumber(num) { |
There was a problem hiding this comment.
isPositiveNumber and isValidPercentage can be hoisted outside of this function to avoid recreating them every time.
There was a problem hiding this comment.
done. Also added tests.
I was considering putting them outside the class but then I can't add tests. Static functions was an option but since instrumentation class has only one instance, static vs non-static was not important here.
|
@jridgewell ptal |
|
LGTM. |
|
When I try to merge this PR I get :
Any idea what is wrong? |
|
No idea, they never pass for me. |
|
I was running a final check locally and found a missing import. Added that. running tests again. will merge from command line once that is done. |
|
@cramforce I can't seem to merge this PR. Can you take a look? Thanks |
Part of #1297