Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
Tweet block - WORK IN PROGESS! Don't merge or review or anything.
  • Loading branch information
notnownikki committed May 10, 2017
commit abc74cd27b048bf9d49a5f7b1db6ce475f78a273
3 changes: 2 additions & 1 deletion blocks/api/categories.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@
*/
const categories = [
{ slug: 'common', title: 'Common Blocks' },
{ slug: 'layout', title: 'Layout Blocks' }
{ slug: 'layout', title: 'Layout Blocks' },
{ slug: 'social', title: 'Social Media' }
];

/**
Expand Down
1 change: 1 addition & 0 deletions blocks/library/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ import './quote';
import './separator';
import './button';
import './pullquote';
import './tweet';
97 changes: 97 additions & 0 deletions blocks/library/tweet/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* Internal dependencies
*/
import { registerBlock, query } from '../../api';
import Sandbox from '../../../components/sandbox';
Copy link
Member

Choose a reason for hiding this comment

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

With the components directory, the idea becomes that we treat these more like external modules, but unlike npm modules, they're first-party in the sense that they'd be WordPress-maintained.

So two things:

  • We can import this from components/sandbox
  • We should move this under a separate WordPress dependencies docblock above this one

There's some more context documented here, which I'm sensing could probably be moved somewhere more obvious:

https://github.com/WordPress/gutenberg/blob/master/docs/coding-guidelines.md#javascript

Also: #716

import Button from '../../../components/button';
import Placeholder from '../../../components/placeholder';

const { prop } = query;

registerBlock( 'core/tweet', {
title: wp.i18n.__( 'Tweet' ),
icon: 'twitter',

category: 'social',

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

Choose a reason for hiding this comment

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

For a case like this, attributes can be defined as a function which receives the raw content of the block as a string, which seems to be exactly what you want:

attributes( url ) {
	return { url };
}

https://github.com/WordPress/gutenberg/tree/master/blocks#wpblocksregisterblock-slug-string-settings-object-

},

edit: class extends wp.element.Component {
constructor() {
super( ...arguments );
this.fetchTweet = this.fetchTweet.bind( this );
this.state = {
url: this.props.attributes.url,
Copy link
Member

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

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

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

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 ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need any of these to be passed as arguments if they're always going to be available at this.attributes.props.url, this.props.setAttributes, and this.setState anyways? (See also related comment below about binding error and success handlers)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, sorry, this is a hangover from when I thought I'd have to set attributes on different components... shouldn't have been left in. Will fix.

setState( { fetching: true, error: false } );
jQuery.ajax( {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this could be related to #691. Generally I tend to discourage network requests in the component itself. The line is blurred here because the result of this request is not really valuable outside the context of the component.

The use of jQuery also sparked my interest. It's handy and readily accessible. We'll probably want to think on our options here some more; probably outside the scope of this pull request specifically.

At the very least, with mind toward #742 and eliminating globals*, we'll probably want to import this as a module and configure Webpack to treat this as an external (reference). I believe it should be enough to add 'jquery': 'jQuery' there, and then you can import jQuery from 'jquery'; at the top of this file.

* For consistency and to enable changing the source from which jQuery is resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this is a little yucky, but it's purely in the editor, and only used to show the tweet as it would appear in the post. The saved html is actually just the tweet's url, as wp will process that anyway, doing the same call this javascript does.

I'll address the globals issue.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also a bit conflicted with relying on jQuery here. It's true we'll have it available in WordPress for the foreseeable future, but coupling an async request with a whole library doesn't feel great for the longer term and portability of editor code.

type: 'GET',
dataType: 'jsonp',
data: {},
Copy link
Member

Choose a reason for hiding this comment

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

Can this be omitted if the object is empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, removing.

timeout: 5000,
Copy link
Member

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

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

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

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 } );
Copy link
Member

Choose a reason for hiding this comment

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

From my earlier comment, you could use an ES2015 arrow function to preserve the this context and enable you to simply call this.setState here.

Copy link
Member

Choose a reason for hiding this comment

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

A big issue with asynchronous behaviors in components is that you can't always be sure that the component is still mounted by the time the network request completes (e.g. if a user removes the block). If that were to occur and this line is called, an error will occur because you're trying to set state on a component that doesn't exist anymore.

A component instance has an isMounted function, but this has been discouraged as an anti-pattern.

In this case, what you will likely want to do is assign the return value of jQuery.ajax into an instance variable and abort the request if the component is unmounted:

componentWillUnmount() {
	if ( this.fetchXHR ) {
		this.fetchXHR.abort();
		delete this.fetchXHR;
	}
}

doFetch() {
	this.fetchXHR = jQuery.ajax( {
		// ...
	} );
}

},
success: function( msg ) {
setAttributes( { url: url } );
Copy link
Member

Choose a reason for hiding this comment

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

Optional, but one nice feature included with ES2015 is object shorthand properties, so this could be written as:

this.props.setAttributes( { url } );

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 ) );
}
}
fetchTweet() {
const { url } = this.state;
this.doFetch( url, this.props.setAttributes, this.setState.bind( this ) );
}
render() {
const { html, url, error, fetching } = this.state;

if ( ! html ) {
Copy link
Member

Choose a reason for hiding this comment

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

In an effort to reduce the indentation of this function*, I'd recommend shifting this around to return early on the case that there is html:

if ( html ) {
	return <Sandbox html={ html } />;
}

* For shorter line-lengths and easier to identify block closings.

return (
<Placeholder instructions={ wp.i18n.__( 'Please paste the URL of the tweet here!' ) } icon="twitter" label={ wp.i18n.__( 'Tweet' ) } className="blocks-tweet">
<div>
<input
type="text"
value={ url }
onChange={ ( event ) => this.setState( { url: event.target.value } ) } />
{ ! fetching ?
(
<Button
isLarge
onClick={ this.fetchTweet }>
{ wp.i18n.__( 'Get Tweet' ) }
</Button>
) : (
<span className="spinner is-active" />
)
}
</div>
{ error && ( <div>{ wp.i18n.__( 'Sorry, we couldn\'t fetch that tweet.' ) }</div> ) }
</Placeholder>
);
}
return (
<Sandbox html={ html } />
);
}
},

save( { attributes } ) {
const { url } = attributes;
return (
<div>{ url }</div>
);
}
} );
46 changes: 46 additions & 0 deletions components/resizable-iframe/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
Resizable Iframe
================

Resizable Iframe is a React component for rendering an `<iframe>` element which can dynamically update its own dimensions using [`window.postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window.postMessage). This is useful in cases where an inline frame of an unknown size is to be displayed on the page.

## Example

The `ResizableIframe` component can be used in much the same way that you would use an `<iframe>` DOM element. Props are automatically transferred to the rendered `<iframe>`, in case you need to specify additional properties or styles.

```html
<ResizableIframe src={ myFrameUrl } frameBorder={ 0 } />
```

## Usage

To allow for resizing of the element, a `ResizableIframe` element listens for `message` events on the `window` object. If the rendered frame is not sandboxed, a script is injected in the frame to manage this behavior on your behalf. If the frame is sandboxed, any page you reference as the `src` URL is responsible for invoking this event using [`window.postMessage`](https://developer.mozilla.org/en-US/docs/Web/API/Window.postMessage). The message should be a JSON string with an `action` value of "resize" and a numeric `width` and `height` to define the new pixel dimensions of the element.

For example, a page can trigger a resize using the following code snippet:

```javascript
if ( window.parent ) {
window.parent.postMessage( JSON.stringify( {
action: 'resize',
width: document.body.clientWidth,
height: document.body.clientHeight
} ), '*' );
}
```

## Props

### `src`

Treated as the `src` URL to be used in the rendered `<iframe>` DOM element.

### `width`

An optional fixed width value, if you don't want this to be the responsibility of the child window.

### `height`

An optional fixed height value, if you don't want this to be the responsibility of the child window.

### `onResize`

An optional function to trigger when the rendered frame has been resized.
193 changes: 193 additions & 0 deletions components/resizable-iframe/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
/**
* Imported from Calypso
*/

/**
* External dependencies
*/
import React from 'react';
Copy link
Member

Choose a reason for hiding this comment

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

We should use our wp.element abstraction consistently, even in forks.

https://github.com/WordPress/gutenberg/tree/master/element

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll change the Sandbox component, too.

import debugFactory from 'debug';
Copy link
Member

Choose a reason for hiding this comment

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

debug is not declared as a dependency. In fact, I'd suggest removing all this debug logic altogether.

import { omit } from 'lodash';
import uuid from 'uuid/v4';

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

export default React.createClass( {
Copy link
Member

Choose a reason for hiding this comment

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

React.createClass is deprecated and this needs to be rewritten as a wp.element.Component class.

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()
Copy link
Member

Choose a reason for hiding this comment

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

What's the value of a randomly generated title?

Copy link
Member Author

Choose a reason for hiding this comment

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

eslint flags an error unless all iframes have a unique title. This seemed the simplest way to get that, unless there's some ID unique to each instance that I can grab? (It wouldn't surprise me, but I couldn't see one)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's a confusing rule description:

<iframe> elements must have a unique title property to indicate its content to the user.

https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/iframe-has-title.md

The value of the rule is not in ensuring guaranteed uniqueness, but rather that users of screen readers are easily able to identify and distinguish the contents of the iframe.

I expect the ESLint rule won't enforce its uniqueness, only that it's present.

To the purpose of the rule, I'd say we should set the title from the Twitter block as something like "Twitter Tweet embed". With that in mind, ResizableIframe should probably accept title as a prop and apply it to the rendered iframe.

The referenced guideline has some more useful context: https://dequeuniversity.com/rules/axe/1.1/frame-title

Copy link
Member

Choose a reason for hiding this comment

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

Maybe also cc @afercia on this one, in case I'm misspeaking. The guidelines do seem to strongly encourage that "no titles are repeated". We could include the Tweet's URL in the title as added uniqueness, but I don't sense this is valuable for screen reader users. The oEmbed response does include some details, like Tweet author and the raw html (from which the tweet's content could be parsed, though not easily).

Copy link
Member Author

Choose a reason for hiding this comment

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

Even the tweet author doesn't help that much, if for example, you're embedded two tweets by the same author... Perhaps "Embedded tweet from " is a good compromise though? It would be easy enough to parse that out of the url.

};
},

componentWillMount: function() {
debug( 'Mounting ' + this.constructor.displayName + ' React component.' );
},

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

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

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

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

maybeConnect: function() {
if ( ! this.isFrameAccessible() ) {
return;
}

const body = this.getFrameBody();
if ( null !== body.getAttribute( 'data-resizable-iframe-connected' ) ) {
return;
}

const script = document.createElement( 'script' );
script.innerHTML = `
( function() {
var observer;

if ( ! window.MutationObserver || ! document.body || ! window.top ) {
return;
}

function sendResize() {
window.top.postMessage( {
action: 'resize',
width: document.body.offsetWidth,
height: document.body.offsetHeight
}, '*' );
}

observer = new MutationObserver( sendResize );
observer.observe( document.body, {
attributes: true,
attributeOldValue: false,
characterData: true,
characterDataOldValue: false,
childList: true,
subtree: true
} );

window.addEventListener( 'load', sendResize, true );

// Hack: Remove viewport unit styles, as these are relative
// the iframe root and interfere with our mechanism for
// determining the unconstrained page bounds.
function removeViewportStyles( ruleOrNode ) {
[ 'width', 'height', 'minHeight', 'maxHeight' ].forEach( function( style ) {
if ( /^\\d+(vmin|vmax|vh|vw)$/.test( ruleOrNode.style[ style ] ) ) {
ruleOrNode.style[ style ] = '';
}
} );
}

Array.prototype.forEach.call( document.querySelectorAll( '[style]' ), removeViewportStyles );
Array.prototype.forEach.call( document.styleSheets, function( stylesheet ) {
Array.prototype.forEach.call( stylesheet.cssRules || stylesheet.rules, removeViewportStyles );
} );

document.body.style.position = 'absolute';
document.body.setAttribute( 'data-resizable-iframe-connected', '' );

sendResize();
} )();
`;
body.appendChild( script );
},

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

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

// Attempt to parse the message data as JSON if passed as string
let data = event.data || {};
if ( 'string' === typeof data ) {
try {
data = JSON.parse( data );
} catch ( e ) {} // eslint-disable-line no-empty
}

// Verify that the mounted element is the source of the message
if ( ! iframe || iframe.contentWindow !== event.source ) {
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;
const { width: oldWidth, height: oldHeight } = this.state;

if ( 'resize' === action && ( oldWidth !== width || oldHeight !== height ) ) {
this.setState( { width, height } );
this.props.onResize();
}
},

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

render: function() {
const omitProps = [ 'onResize' ];
if ( ! this.props.src ) {
omitProps.push( 'src' );
}
return (
<iframe
ref={ ( node ) => this.iframe = node }
title={ this.props.title }
seamless="seamless"
scrolling="no"
{ ...omit( this.props, omitProps ) }
onLoad={ this.onLoad }
width={ this.props.width || this.state.width }
height={ this.props.height || this.state.height } />
);
}
} );
Loading