Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Make tweet id a bindable attribute #23953

Merged
merged 2 commits into from
Aug 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions extensions/amp-bind/0.1/bind-validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,9 @@ function createElementRules_() {
'datetime': null,
'title': null,
},
'AMP-TWITTER': {
'data-tweetid': null,
},
'AMP-VIDEO': {
'alt': null,
'attribution': null,
Expand Down
5 changes: 5 additions & 0 deletions extensions/amp-bind/amp-bind.md
Original file line number Diff line number Diff line change
Expand Up @@ -488,6 +488,11 @@ Only binding to the following components and attributes are allowed:
<td><code>[src]</code></td>
<td>Fetches JSON from the new URL and merges it into the existing state. <em>Note the following update will ignore <code>&lt;amp-state&gt;</code> elements to prevent cycles.</em></td>
</tr>
<tr>
<td><code>&lt;amp-twitter&gt;</code></td>
<td><code>[data-tweetid]</code></td>
<td>Changes the displayed Tweet.</td>
</tr>
<tr>
<td><code>&lt;amp-video&gt;</code></td>
<td><code>[alt]</code><br><code>[attribution]</code><br><code>[controls]</code><br><code>[loop]</code><br><code>[poster]</code><br><code>[preload]</code><br><code>[src]</code></td>
Expand Down
21 changes: 20 additions & 1 deletion extensions/amp-twitter/0.1/amp-twitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ class AmpTwitter extends AMP.BaseElement {
*/
updateForSuccessState_(height) {
this.mutateElement(() => {
this.toggleLoading(false);
if (this.userPlaceholder_) {
this.togglePlaceholder(false);
}
Expand All @@ -128,7 +129,7 @@ class AmpTwitter extends AMP.BaseElement {
}

/**
* Updates wheen the tweet that failed to load. This uses the fallback
* Updates when the tweet failed to load. This uses the fallback
* provided if available. If not, it uses the user specified placeholder.
* @private
*/
Expand All @@ -137,6 +138,7 @@ class AmpTwitter extends AMP.BaseElement {
const content = fallback || this.userPlaceholder_;

this.mutateElement(() => {
this.toggleLoading(false);
if (fallback) {
this.togglePlaceholder(false);
this.toggleFallback(true);
Expand All @@ -148,6 +150,14 @@ class AmpTwitter extends AMP.BaseElement {
});
}

/**
* amp-twitter reuses the loading indicator when id changes via bind mutation
* @override
*/
isLoadingReused() {
return true;
}

/** @override */
createLoaderLogoCallback() {
const html = htmlFor(this.element);
Expand Down Expand Up @@ -182,6 +192,15 @@ class AmpTwitter extends AMP.BaseElement {
}
return true;
}

/** @override */
mutatedAttributesCallback(mutations) {
if (this.iframe_ && mutations['data-tweetid'] != null) {
this.unlayoutCallback();
this.toggleLoading(true, /* opt_force */ true);
this.layoutCallback();
}
}
}

AMP.extension('amp-twitter', '0.1', AMP => {
Expand Down
12 changes: 12 additions & 0 deletions extensions/amp-twitter/0.1/test/test-amp-twitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,18 @@ describes.realWin(
});
});

it('should replace iframe after tweetid mutation', () => {
const newTweetId = '638793490521001985';
return getAmpTwitter(tweetId).then(ampTwitter => {
const spy = sandbox.spy(ampTwitter.implementation_, 'toggleLoading');
const iframe = ampTwitter.querySelector('iframe');
ampTwitter.setAttribute('data-tweetid', newTweetId);
ampTwitter.mutatedAttributesCallback({'data-tweetid': newTweetId});
expect(spy).to.be.calledWith(true);
expect(ampTwitter.querySelector('iframe')).to.not.equal(iframe);
});
});

describe('cleanupTweetId_', () => {
it('does not affect valid tweet ids', () => {
const good = '585110598171631616';
Expand Down