✨ amp-facebook-comments: Initial Bento component#33852
✨ amp-facebook-comments: Initial Bento component#33852caroqliu merged 15 commits intoampproject:mainfrom
Conversation
| ref={ref} | ||
| ready={!!name} | ||
| sandbox={sandbox} | ||
| sandbox={excludeSandbox ? undefined : sandbox} |
There was a problem hiding this comment.
Sigh, I was going to suggest using sandbox={null}, but it seems Preact will incorrectly coerce that to an empty string.
There was a problem hiding this comment.
Should we file an issue with Preact?
There was a problem hiding this comment.
Yes, if the behaviour is different than other JSX syntax environments.
kristoferbaxter
left a comment
There was a problem hiding this comment.
Left a few small items to consider.
| /* non-overridable props */ | ||
| // We sandbox all 3P iframes however facebook embeds completely break in | ||
| // sandbox mode since they need access to document.domain, so we | ||
| // exclude facebook. |
There was a problem hiding this comment.
Is the only thing they need access to is document.domain? Could we create a Proxy?
There was a problem hiding this comment.
@jridgewell Do you have more historical context here? This behavior mimics:
Lines 158 to 161 in 2bddbc2
And comment is copied from test page of 0.1:
amphtml/extensions/amp-facebook/0.1/test/test-amp-facebook.js
Lines 72 to 74 in 2bddbc2
There was a problem hiding this comment.
We might try adding sandbox again, and see if it breaks. Maybe Facebook has updated their scripts.
| ref={ref} | ||
| ready={!!name} | ||
| sandbox={sandbox} | ||
| sandbox={excludeSandbox ? undefined : sandbox} |
There was a problem hiding this comment.
Yes, if the behaviour is different than other JSX syntax environments.
| /* non-overridable props */ | ||
| // We sandbox all 3P iframes however facebook embeds completely break in | ||
| // sandbox mode since they need access to document.domain, so we | ||
| // exclude facebook. |
There was a problem hiding this comment.
We might try adding sandbox again, and see if it breaks. Maybe Facebook has updated their scripts.
* Prototype * Add note about excludeSandbox * Add unit tests * Add types * Separate option props with default locale * Prettify bundles config * Use Object.assign * Remove constructor * Update imports * Assert on given `allow` * Simplify onReady condition * Allow facebook loader dependency * Update test url
Note: The
amp-facebook-*components rely on component tag name to identify which embedded post type to use:amphtml/3p/facebook.js
Lines 213 to 225 in 64159c5
This PR adds the Bento component
1.0version foramp-facebook-commentswithout modifying the above. A follow up PR should clean up the above conditions to betagNameindependent, perhaps introducing an enum instead which the Preact component can use. (Currently it is not ideal for the Preact layer to have to pass incontextOptions={{tagName: 'AMP-FACEBOOK-COMMENTS'}})