Added percentageVisible{Max,Min} conditions for visibilitySpec. #2881
Conversation
| // TODO(avimehta, #1297): Add all the listeners so that visibility | ||
| // conditions are monitored and callback is called when the conditions | ||
| // are met. | ||
| if (!config['visibilitySpec']) { |
There was a problem hiding this comment.
Mind flipping the conditionals? !else double-negatives make everything confusing.
|
ptal. I am planning to add a few more tests but other stuff is ready for review. |
| let VisibilityListenerCallbackDef; | ||
|
|
||
| /** | ||
| * @typedef {Object.<string, JSONObject|VisibilityListenerCallbackDef>} |
There was a problem hiding this comment.
This always seems to be a VisibilityListenerDef.
There was a problem hiding this comment.
Config is a json object. I add it to the object at https://github.com/ampproject/amphtml/pull/2881/files#diff-6936d113e81d9bf7788ab8e145841720R88
There was a problem hiding this comment.
The | is the confusing part then. Can you create a VisibilityListenerConfigDef?
|
|
||
| this.registerForViewportEvents_(); | ||
|
|
||
| this.listeners_[resId()] = (this.listeners_[resId()] || []); |
There was a problem hiding this comment.
This'll error, resId isn't a function.
There was a problem hiding this comment.
Weird. I swear I ran the tests. will fix.
|
LGTM. |
Part of #1297
Tests to follow.