Subscriptions: Add action execution and rendering actions #13790
Conversation
prateekbh
commented
Mar 3, 2018
prateekbh
commented
- Adds event listener for actions
- Adds rendering logic for platforms
- Adds methods for Subscription platform Interface
…mp-subs-actions
…mp-subs-actions
…mp-subs-action-listener
| this.ampdoc_ = ampdoc; | ||
|
|
||
| /** @private @const */ | ||
| this.document_ = this.ampdoc_.win.document; |
There was a problem hiding this comment.
You should almost never use document. Instead, there's ampdoc.getRootNode() and family.
| const expr = candidate.getAttribute('subscriptions-display'); | ||
| if (expr && evaluateExpr(expr, entitlements)) { | ||
| candidate.setAttribute('i-amphtml-subs-display', ''); | ||
| } |
There was a problem hiding this comment.
If you do else {candidate.removeAttribute(...)} this method will become reentrant.
There was a problem hiding this comment.
I believe that this should be a part of a separate deactivate method which would be called for all platforms which are not selected. WDYT?
There was a problem hiding this comment.
Probably a good idea. Feel free to proceed with that idea in a separate PR.
| * @private | ||
| */ | ||
| initializeListeners_() { | ||
| this.document_.addEventListener('click', e => { |
There was a problem hiding this comment.
Ditto. Only add listeners to ampdoc.getRootNode()
| const actionExecution = this.actions_.execute(action); | ||
| actionExecution.then(result => { | ||
| if (result) { | ||
| this.getEntitlements(); |
There was a problem hiding this comment.
Would this have any affect on rendering or such? Also, you may want to execute this call via the top-level service b/c changing of entitlements here could result in selecting a new platform... Maybe something like: service.reAuthorizePlatform(this) or such?
There was a problem hiding this comment.
yes, added a todo which will resolve with the actio delegation PR as both need a service interface
| * Renders the UI specific to the platform | ||
| */ | ||
| render() { | ||
|
|
| /** | ||
| * Renders the UI specific to the platform | ||
| */ | ||
| render() { |
There was a problem hiding this comment.
Implement right away on google subscriptions: even if it's just an empty method. But I feel like the name is not broad enough. Maybe activate()?
…mp-subs-action-listener
…#13790) * fix animating circle fixed position * adding selectPlatform method * WIP actions * adding tests * fixing comments * WIP action delegation * renders the platform actions * adding tests * fixing comments
