Skip to content

Commit

Permalink
Force the 'data-embed-as' attribute to 'video' and make sure to show …
Browse files Browse the repository at this point in the history
…post text (#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
  • Loading branch information
nainar authored and cvializ committed Feb 22, 2018
1 parent 5546290 commit ba7ee25
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 1 deletion.
13 changes: 12 additions & 1 deletion 3p/facebook.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,21 @@ function getPostContainer(global, data) {
setStyle(c, 'text-align', 'center');
}
const container = global.document.createElement('div');
const embedAs = data.embedAs || 'post';
let embedAs = data.embedAs || 'post';
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);
// If the user hasn't set the `data-embed-as` attribute and the provided href
// is a video, Force the `data-embed-as` attribute to 'video' and make sure
// to show the post's text.
if (data.href.match(/\/videos\/\d+\/?$/) &&
!container.hasAttribute('data-embed-as')) {
embedAs = 'video';
container.setAttribute('data-embed-as', 'video');
// Since 'data-embed-as="video"' disables post text, setting the 'data-show-text'
// to 'true' enables the ability to see the text (changed from the default 'false')
container.setAttribute('data-show-text', 'true');
}
container.className = 'fb-' + embedAs;
container.setAttribute('data-href', data.href);
return container;
Expand Down
1 change: 1 addition & 0 deletions build-system/amp.extern.js
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ data.hideCta;
data.smallHeader;
data.adaptContainerWidth;
data.showFacePile;
data.showText;

// 3p code
var twttr;
Expand Down
22 changes: 22 additions & 0 deletions extensions/amp-facebook/0.1/test/test-amp-facebook.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,28 @@ describes.realWin('amp-facebook', {
expect(fbVideo.getAttribute('data-href')).to.equal(fbVideoHref);
});

it('adds fb-video element with `data-embed-as` and `data-show-text` ' +
'attributes set correctly', () => {
const div = doc.createElement('div');
div.setAttribute('id', 'c');
doc.body.appendChild(div);
win.context = {
tagName: 'AMP-FACEBOOK',
};

facebook(win, {
href: fbVideoHref,
width: 111,
height: 222,
});
const fbVideo = doc.body.getElementsByClassName('fb-video')[0];
expect(fbVideo).not.to.be.undefined;
expect(fbVideo.classList.contains('fb-video')).to.be.true;
expect(fbVideo.getAttribute('data-embed-as')).to.equal('video');
expect(fbVideo.getAttribute('data-show-text')).to.equal('true');
});


it('removes iframe after unlayoutCallback', () => {
return getAmpFacebook(fbPostHref).then(ampFB => {
const iframe = ampFB.querySelector('iframe');
Expand Down

0 comments on commit ba7ee25

Please sign in to comment.