Force the 'data-embed-as' attribute to 'video' and make sure to show post text #13454
Conversation
|
@cvializ could you take a look at the PR? |
3p/facebook.js
Outdated
| user().assert(['post', 'video'].indexOf(embedAs) !== -1, | ||
| 'Attribute data-embed-as for <amp-facebook> value is wrong, should be' + | ||
| ' "post" or "video" was: %s', embedAs); | ||
| // Force the `data-embed-as` attribute to 'video' and make sure to show the post's text. | ||
| if (data.href.indexOf('/videos/') > 0) { | ||
| embedAs = 'video'; |
There was a problem hiding this comment.
Hmm, the behavior of this could be strange to a publisher if they explicitly set data-embed-as="post" but this auto-changes it to video. Is it always better the change the value to video because the user experience is broken with post-videos?
There was a problem hiding this comment.
Agree, maybe in addition to the RegEx check, we also check if data.embedAs is falsey meaning user did not explicitly specify a type?
There was a problem hiding this comment.
Good point, made the change.
3p/facebook.js
Outdated
| user().assert(['post', 'video'].indexOf(embedAs) !== -1, | ||
| 'Attribute data-embed-as for <amp-facebook> value is wrong, should be' + | ||
| ' "post" or "video" was: %s', embedAs); | ||
| // Force the `data-embed-as` attribute to 'video' and make sure to show the post's text. | ||
| if (data.href.indexOf('/videos/') > 0) { |
There was a problem hiding this comment.
This would generate false positives if Facebook username has the word videos in it. Would need to tighten up the regex to only match */(videos)/d+
3p/facebook.js
Outdated
| user().assert(['post', 'video'].indexOf(embedAs) !== -1, | ||
| 'Attribute data-embed-as for <amp-facebook> value is wrong, should be' + | ||
| ' "post" or "video" was: %s', embedAs); | ||
| // Force the `data-embed-as` attribute to 'video' and make sure to show the post's text. | ||
| if (data.href.indexOf('/videos/') > 0) { | ||
| embedAs = 'video'; |
There was a problem hiding this comment.
Agree, maybe in addition to the RegEx check, we also check if data.embedAs is falsey meaning user did not explicitly specify a type?
3p/facebook.js
Outdated
| if (data.href.indexOf('/videos/') > 0) { | ||
| embedAs = 'video'; | ||
| container.setAttribute('data-embed-as', 'video'); | ||
| container.setAttribute('data-show-text', 'true'); |
There was a problem hiding this comment.
add a comment why show-text must be true for video. The reason is not obvious
3p/facebook.js
Outdated
| if (data.href.indexOf('/videos/') > 0) { | ||
| embedAs = 'video'; | ||
| container.setAttribute('data-embed-as', 'video'); | ||
| container.setAttribute('data-show-text', 'true'); |
3p/facebook.js
Outdated
| user().assert(['post', 'video'].indexOf(embedAs) !== -1, | ||
| 'Attribute data-embed-as for <amp-facebook> value is wrong, should be' + | ||
| ' "post" or "video" was: %s', embedAs); | ||
| // Force the `data-embed-as` attribute to 'video' and make sure to show the post's text. | ||
| if (data.href.indexOf('/videos/') > 0) { |
3p/facebook.js
Outdated
| user().assert(['post', 'video'].indexOf(embedAs) !== -1, | ||
| 'Attribute data-embed-as for <amp-facebook> value is wrong, should be' + | ||
| ' "post" or "video" was: %s', embedAs); | ||
| // Force the `data-embed-as` attribute to 'video' and make sure to show the post's text. | ||
| if (data.href.indexOf('/videos/') > 0) { | ||
| embedAs = 'video'; |
There was a problem hiding this comment.
Good point, made the change.
nainar
force-pushed
the
amp_facebook_embed
branch
from
287147a to
d6e6d01
Compare
|
@aghassemi could you merge the CL? |
…post text (ampproject#13454) * Force the 'data-embed-as' attribute to 'video' and make sure to show the post's text. * Strengthen regex and force data-embed-as attribute when not specifically set
Fixes #12088
It does so by:
data-embed-asfor all videos to 'video'data-show-textto 'true' (changed from the default 'false')Initially the issue would repro when switching from desktop to mobile mode in Chrome devtools as well.