Fixing embedded pin render issues #1062
Conversation
This fixes problems where the renderable context is not `this` instead of the renderPin() function. Also, promise to load `structure`, not `image`.
|
ping @erwinmombay |
| @@ -350,7 +350,7 @@ class AmpPinterest extends AMP.BaseElement { | |||
| '?pin_ids=' + pinId + | |||
| '&sub=www&base_scheme=https'; | |||
| return call(query).then(r => { | |||
There was a problem hiding this comment.
we can probably just do:
return call(query).then(renderPin.bind(this));which would get rid of the self param as well.
There was a problem hiding this comment.
or change renderPin into an arrow function.
const renderPin = (r) => {
this.element...
}
call(query).then(renderPin);then we wouldn't need the bind.
i'm ok with either solution.
There was a problem hiding this comment.
@erwinmombay sorry, but I can't make either of these suggestions work. Help would be welcome, thanks!
There was a problem hiding this comment.
@kentbrew this seems to work https://github.com/ampproject/amphtml/pull/1112/files can you try it out and see if it works for you here? if it still doesnt, let me know and lets go with your fix since getting the fix out is probably higher priority.
|
@cramforce @dvoytenko are you guys ok with us merging this so that we can deploy a new release with an unbroken pinterest component. the review on #1119 might take a bit longer. |
|
@erwinmombay Yeah, I'm ok with this. |
|
LGTM. |
Fixing embedded pin render issues
Also fixed a remaining stray reference to iframes in the docs.