Skip to content

Add tweet block for embedding tweets #754

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.

Sign up for GitHub

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

Closed
notnownikki wants to merge 13 commits into from
Closed

Add tweet block for embedding tweets #754

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3cd38d1
Tweet block - WORK IN PROGESS! Don't merge or review or anything.
notnownikki May 10, 2017
abc74cd
Tweet block - WORK IN PROGESS! Don't merge or review or anything.
notnownikki May 10, 2017
5fa3cb8
Polish placeholder error message.
jasmussen May 10, 2017
8c38062
Merge branch 'add/twitter-block' of github.com:WordPress/gutenberg in…
jasmussen May 10, 2017
09814e5
Merge latest changes.
jasmussen May 10, 2017
eedb7d7
Tweet block
notnownikki May 10, 2017
ce8fc70
Fixes from code review
notnownikki May 11, 2017
5fd1bb9
Lint fixes
notnownikki May 11, 2017
0aad93d
Add jQuery
notnownikki May 11, 2017
5ce13f6
i18n fix for sandboxed iframe titles
notnownikki May 11, 2017
a43001f
Merge branch 'master' into add/twitter-block
notnownikki May 11, 2017
c223f18
Fixed iframe title after some a11y discussions
notnownikki May 11, 2017
ff12c83
Merge branch 'add/twitter-block' of github.com:WordPress/gutenberg in…
notnownikki May 11, 2017
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
Prev Previous commit
Next Next commit
Fixes from code review
  • Loading branch information
@notnownikki
notnownikki committed May 11, 2017
commit ce8fc709df75f58719595cd3dda172d694445b4b
2 changes: 1 addition & 1 deletion blocks/library/embed/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ registerBlock( 'core/embed', {
if ( ! url ) {
return (
<Placeholder icon="cloud" label={ wp.i18n.__( 'Embed URL' ) } className="blocks-embed">
<input type="url" className="components-placeholder__input" placeholder={ wp.i18n.__( 'Enter URL to embed...' ) } />
<input type="url" className="components-placeholder__input" placeholder={ wp.i18n.__( 'Enter URL to embed…' ) } />
<Button isLarge>
{ wp.i18n.__( 'Embed' ) }
</Button>
Expand Down
81 changes: 50 additions & 31 deletions blocks/library/tweet/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
/**
* External dependencies
*/
import jQuery from 'jquery';
/**
* WordPress dependencies
*/
import Sandbox from 'components/sandbox';
import Button from 'components/button';
import Placeholder from 'components/placeholder';
/**
* Internal dependencies
*/
import { registerBlock, query } from '../../api';
import Sandbox from '../../../components/sandbox';
import Button from '../../../components/button';
import Placeholder from '../../../components/placeholder';

const { prop } = query;

Expand All @@ -14,55 +21,72 @@ registerBlock( 'core/tweet', {

category: 'social',

attributes: {
url: prop( '*', 'innerHTML' ), // our html is just a div with the url in, for WP's oembed to process
attributes( url ) {
return { url };
},

edit: class extends wp.element.Component {
constructor() {
super( ...arguments );
this.fetchTweet = this.fetchTweet.bind( this );
// Copies the block's url so we can edit it without having the block
// update (i.e. refetch the tweet) every time it changes in this edit component.
this.state = {
url: this.props.attributes.url,
Copy link
Member

@aduth aduth May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's discouraged to create copies of props in state because it muddies the fact that the source of truth is the prop itself, leaving you responsible for maintaining when that prop changes. And evidenced by a lack of componentWillReceiveProps or componentDidUpdate, it appears we're not.

Instead, where you're currently using this.state.url, you should try to reference this.props.attributes.url directly instead.

And depending on if we want the component to trigger another request if the URL ever changes, it'd be good to bind to componentDidUpdate to check if the URL has changed and fire doFetch again.

Copy link
Member Author

@notnownikki notnownikki May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the button is clicked (or form submitted) I'd use a reference to the text input to set the url, instead of having it set when the text input changes?

Copy link
Member

@aduth aduth May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when the button is clicked (or form submitted) I'd use a reference to the text input to set the url, instead of having it set when the text input changes?

Ah, I think I overlooked that. With that in mind, it can be fine to create a copy.

Here's an archived (some outdated syntax) article which explains more of the context and highlights this exception:

https://github.com/facebook/react/blob/8cac523/docs/tips/10-props-in-getInitialState-as-anti-pattern.md

However, it's not an anti-pattern if you make it clear that the prop is only seed data for the component's internally-controlled state:

Since we don't have as much control over the naming of the incoming prop, except if we were to create an intermediary component or rename the attribute (both of which don't seem ideal), I think it'd be fine to ignore the initialUrl recommendation here.

Copy link
Member Author

@notnownikki notnownikki May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment in the constructor to make it clear what we're doing here.

html: '',
error: false,
fetching: false,
};
}
doFetch( url, setAttributes, setState ) {
setState( { fetching: true, error: false } );
jQuery.ajax( {
doFetch( url ) {
this.setState( { fetching: true, error: false } );
this.fetchXHR = jQuery.ajax( {
type: 'GET',
dataType: 'jsonp',
data: {},
timeout: 5000,
Copy link
Member

@aduth aduth May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the default timeout value? Do we really need to specify this?

Copy link
Member Author

@notnownikki notnownikki May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately we do need the timeout.

JSONP requests are horrible. If they fail, they don't call the error callback, so the only way to do it is to specify a large-ish timeout and wait for it to fail.

Copy link
Member

@aduth aduth May 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But is the issue of wanting to avoid waiting a long time for requests specific to this block? I'm curious if and what the default timeout is, or if this should be applied globally through $.ajaxSetup, or whether there's any consensus in WordPress JavaScript that we even need impose a timeout.

More a consistency question than anything.

Copy link
Member Author

@notnownikki notnownikki May 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to impose a timeout, or the error handling simply does not work. It shouldn't be applied globally though ajaxSetup because this is specific to JSONP requests.

From http://api.jquery.com/jQuery.ajax/

Note: This handler is not called for cross-domain script and cross-domain JSONP requests.

There doesn't seem to be a default timeout value.

The other way of dealing with errors for cross domain requests is a jQuery plugin, https://github.com/jaubourg/jquery-jsonp , but it doesn't seem to be maintained.

url: 'https://publish.twitter.com/oembed?url=' + encodeURI( url ),
error: function() {
setState( { fetching: false, error: true } );
},
success: function( msg ) {
setAttributes( { url: url } );
setState( { fetching: false, error: false, html: msg.html } );
error: () => this.setState( { fetching: false, error: true } ),
success: ( msg ) => {
this.props.setAttributes( { url } );
this.setState( { fetching: false, error: false, html: msg.html } );
},
} );
}
componentDidMount() {
if ( this.state.url ) {
this.doFetch( this.state.url, this.props.setAttributes, this.setState.bind( this ) );
this.doFetch( this.state.url );
}
}
componentWillUnmount() {
if ( this.fetchXHR ) {
this.fetchXHR.abort();
delete this.fetchXHR;
}
}
fetchTweet() {
fetchTweet( event ) {
const { url } = this.state;
this.doFetch( url, this.props.setAttributes, this.setState.bind( this ) );
event.preventDefault();
this.doFetch( url );
}
render() {
const { html, url, error, fetching } = this.state;

if ( ! html ) {
if ( html ) {
const author = this.state.url.split( '/' )[3];
/* translators: {AUTHOR}: username of the tweet's author */
const __title = wp.i18n.__( 'Tweet from {AUTHOR}' );
const title = __title.replace( '{AUTHOR}', author );
return (
<Placeholder icon="twitter" label={ wp.i18n.__( 'Twitter' ) } className="blocks-tweet">
<Sandbox
html={ html }
title={ title } />
);
}

return (
<Placeholder icon="twitter" label={ wp.i18n.__( 'Twitter' ) } className="blocks-tweet">
<form onSubmit={ this.fetchTweet }>
<input
type="text"
className="components-placeholder__input"
value={ url }
placeholder={ wp.i18n.__( 'Enter tweet URL to embed...' ) }
Expand All @@ -71,27 +95,22 @@ registerBlock( 'core/tweet', {
(
<Button
isLarge
onClick={ this.fetchTweet }>
type="submit">
{ wp.i18n.__( 'Embed' ) }
</Button>
) : (
<span className="spinner is-active" />
)
}
{ error && ( <p className="components-placeholder__error">{ wp.i18n.__( 'Sorry, we couldn\'t fetch that tweet.' ) }</p> ) }
</Placeholder>
);
}
return (
<Sandbox html={ html } />
</form>
{ error && ( <p className="components-placeholder__error">{ wp.i18n.__( 'Sorry, we couldn\'t fetch that tweet.' ) }</p> ) }
</Placeholder>
);
}
},

save( { attributes } ) {
const { url } = attributes;
return (
<div>{ url }</div>
);
return url;
}
} );
98 changes: 38 additions & 60 deletions components/resizable-iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,69 +5,48 @@
/**
* External dependencies
*/
import React from 'react';
import debugFactory from 'debug';
import { omit } from 'lodash';
import uuid from 'uuid/v4';

/**
* Globals
*/
const debug = debugFactory( 'gutenburg:resizable-iframe' ),
noop = () => {};

export default React.createClass( {
displayName: 'ResizableIframe',

propTypes: {
src: React.PropTypes.string,
width: React.PropTypes.oneOfType( [
React.PropTypes.string,
React.PropTypes.number
] ),
height: React.PropTypes.oneOfType( [
React.PropTypes.string,
React.PropTypes.number
] ),
onLoad: React.PropTypes.func,
onResize: React.PropTypes.func,
title: React.PropTypes.string
},

getInitialState: function() {
return { width: 0, height: 0 };
},

getDefaultProps: function() {
return {
onLoad: noop,
onResize: noop,
title: uuid()
export default class ResizableIframe extends wp.element.Component {

constructor() {
super( ...arguments );
this.state = {
width: 0,
height: 0
};
},
this.getFrameBody = this.getFrameBody.bind( this );
this.maybeConnect = this.maybeConnect.bind( this );
this.isFrameAccessible = this.isFrameAccessible.bind( this );
this.checkMessageForResize = this.checkMessageForResize.bind( this );
}

componentWillMount: function() {
debug( 'Mounting ' + this.constructor.displayName + ' React component.' );
},
static get defaultProps() {
return {
onLoad: () => {},
onResize: () => {},
title: ''
};
}

componentDidMount: function() {
componentDidMount() {
window.addEventListener( 'message', this.checkMessageForResize, false );
this.maybeConnect();
},
}

componentDidUpdate: function() {
componentDidUpdate() {
this.maybeConnect();
},
}

componentWillUnmount: function() {
componentWillUnmount() {
window.removeEventListener( 'message', this.checkMessageForResize );
},
}

getFrameBody: function() {
getFrameBody() {
return this.iframe.contentDocument.body;
},
}

maybeConnect: function() {
maybeConnect() {
if ( ! this.isFrameAccessible() ) {
return;
}
Expand Down Expand Up @@ -129,17 +108,17 @@ export default React.createClass( {
} )();
`;
body.appendChild( script );
},
}

isFrameAccessible: function() {
isFrameAccessible() {
try {
return !! this.getFrameBody();
} catch ( e ) {
return false;
}
},
}

checkMessageForResize: function( event ) {
checkMessageForResize( event ) {
const iframe = this.iframe;

// Attempt to parse the message data as JSON if passed as string
Expand All @@ -155,8 +134,6 @@ export default React.createClass( {
return;
}

debug( 'Received message: %o', data );

// Update the state only if the message is formatted as we expect, i.e.
// as an object with a 'resize' action, width, and height
const { action, width, height } = data;
Expand All @@ -166,15 +143,16 @@ export default React.createClass( {
this.setState( { width, height } );
this.props.onResize();
}
},
}

onLoad: function( event ) {
onLoad( event ) {
this.maybeConnect();
this.props.onLoad( event );
},
}

render: function() {
render() {
const omitProps = [ 'onResize' ];

if ( ! this.props.src ) {
omitProps.push( 'src' );
}
Expand All @@ -190,4 +168,4 @@ export default React.createClass( {
height={ this.props.height || this.state.height } />
);
}
} );
};
28 changes: 9 additions & 19 deletions components/sandbox/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,18 @@
/**
* External dependencies
*/
import React from 'react';

/**
* Internal dependencies
*/
import ResizableIframe from 'components/resizable-iframe';

const Sandbox = React.createClass( {
displayName: 'Sandbox',

propTypes: {
html: React.PropTypes.string,
},
export default class Sandbox extends wp.element.Component {

getDefaultProps: function() {
static get defaultProps() {
return {
html: '',
title: '',
};
},
}

componentDidMount: function() {
componentDidMount() {
const body = this.node.getFrameBody();
const { html } = this.props;

Expand All @@ -42,15 +33,14 @@ const Sandbox = React.createClass( {
for ( let i = 0; i < newscripts.length; i++ ) {
body.appendChild( newscripts[ i ] );
}
},
}

render: function() {
render() {
return (
<ResizableIframe
sandbox="allow-same-origin allow-scripts"
title={ this.props.title }
ref={ ( node ) => this.node = node } />
);
}
} );

export default Sandbox;
};
2 changes: 1 addition & 1 deletion post-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ window._wpGutenbergPost = {
'<!-- /wp:core/embed -->',

'<!-- wp:core/tweet -->',
'<div>https://twitter.com/automattic/status/777261837335326720</div>',
'https://twitter.com/automattic/status/777261837335326720',
'<!-- /wp:core/tweet -->',
].join( '' ),
},
Expand Down
3 changes: 2 additions & 1 deletion webpack.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ const config = {
externals: {
react: 'React',
'react-dom': 'ReactDOM',
'react-dom/server': 'ReactDOMServer'
'react-dom/server': 'ReactDOMServer',
'jquery': 'jQuery'
},
resolve: {
modules: [
Expand Down