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

Enhance the embed block to support all WP_oEmbed embeds #816

Merged
merged 16 commits into from
Jun 2, 2017
Merged
Show file tree
Hide file tree
Changes from 15 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: 2 additions & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
}
},
"globals": {
"wp": true
"wp": true,
"wpApiSettings": true
},
"plugins": [
"react",
Expand Down
149 changes: 113 additions & 36 deletions blocks/library/embed/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { Button, Placeholder } from 'components';
import { Button, Placeholder, HtmlEmbed, Spinner } from 'components';

/**
* Internal dependencies
Expand Down Expand Up @@ -34,7 +34,6 @@ registerBlock( 'core/embed', {
category: 'embed',

attributes: {
url: attr( 'iframe', 'src' ),
title: attr( 'iframe', 'title' ),
caption: children( 'figcaption' ),
},
Expand Down Expand Up @@ -73,52 +72,130 @@ registerBlock( 'core/embed', {
}
},

edit( { attributes, setAttributes, focus, setFocus } ) {
const { url, title, caption } = attributes;
edit: class extends wp.element.Component {
constructor() {
super( ...arguments );
this.doServerSideRender = this.doServerSideRender.bind( this );
this.state = {
html: '',
type: '',
error: false,
fetching: false,
};
this.noPreview = [
'facebook.com',
];
if ( this.props.attributes.url ) {
// if the url is already there, we're loading a saved block, so we need to render
this.doServerSideRender();
}
}

if ( ! url ) {
return (
<Placeholder icon="cloud" label={ wp.i18n.__( 'Embed URL' ) } className="blocks-embed">
<input type="url" className="placeholder__input" placeholder={ wp.i18n.__( 'Enter URL to embed here...' ) } />
<Button isLarge>
{ wp.i18n.__( 'Embed' ) }
</Button>
</Placeholder>
componentWillUnmount() {
// can't abort the fetch promise, so let it know we will unmount
this.unmounting = true;
}

doServerSideRender( event ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we extract this ServerSideRender to a HoC? Maybe something like:

<ServerSideBlock blockType={ blockType } arguments={ arguments }>
  { ( html, error ) => (
    <Sandbox html={ html } /> 
  ) }
</ServerSideBlock>

Any blocker to achieve 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.

The only blocker is I need to read more React docs to figure out how to do it. There were async issues with having Sandbox as the containing block, this seems like a much better way, I've just never done it before.

Copy link
Contributor

Choose a reason for hiding this comment

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

if ( event ) {
event.preventDefault();
}
const { url } = this.props.attributes;
const api_url = wpApiSettings.root + 'oembed/1.0/proxy?url=' + encodeURIComponent( url ) + '&_wpnonce=' + wpApiSettings.nonce;

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

Choose a reason for hiding this comment

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

When loading the page on master now, I see the following warning from this line:

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so this needs to be moved into componentWillMount?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe. I'd have to look again at where this function is called originally, and if by componentDidMount, why the component would not be mounted at this point.

To a more general point, I think this is a handy reference for when side effects and state changes should occur during component lifecycle:

https://gist.github.com/bvaughn/923dffb2cd9504ee440791fade8db5f9

fetch( api_url, {
Copy link
Member

Choose a reason for hiding this comment

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

There is an oEmbed Proxy endpoint now in core which should be used, I believe. See https://core.trac.wordpress.org/ticket/40450

Copy link
Member

Choose a reason for hiding this comment

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

For me it fails to embed because this is not the proper path. Needs path to the WP site. https://developer.wordpress.org/reference/functions/rest_url/

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I hardcoded the api url here because I didn't know how to discover it. I see that function is how to do it in PHP, is there a JS version too?

Copy link
Member

Choose a reason for hiding this comment

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

You'll need the URL and the nonce alike. They can be exported from PHP to JS as was done for Media: https://github.com/WordPress/wordpress-develop/blob/255bd917f2bc3c0d2b324ebbc979ab4147631c06/src/wp-includes/media.php#L3421-L3431

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'm not sure how to access that from JS... do you have an example of the JS side you could link me to?

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to get a new one. In WordPress, a nonce is not strictly a number-used-once. It is more like a CSRF token that will expire after a day. You can presume that the WP-API should be responsible for keeping wpApiSettings.nonce updated, though I don't think it is currently.

Copy link
Member

Choose a reason for hiding this comment

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

@notnownikki Alternatively, if you need other PHP settings, you can use https://developer.wordpress.org/reference/functions/wp_localize_script/ in WordPress.

Copy link
Contributor

@youknowriad youknowriad May 23, 2017

Choose a reason for hiding this comment

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

I was thinking we could use the proxy endpoint as well but I like the idea of having a more generic server side rendering endpoint (like the one implemented in the PR) 

Edit: I guess we're interested in the video flag returned by the proxy endpoint.

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'm having a few problems implementing this...

If I fetch the URL resulting from

wpApiSettings.root + 'oembed/1.0/proxy?url=' + encodeUriComponent( url ) + '&_wpnonce=' + wpApiSettings.nonce

...then I get back "Cookie nonce is invalid". (I've tried to set the none in a header too, but same error.)

However, if I visit the URL in a browser, I don't get a nonce error, but I do get a 404, the route isn't found.

Can you point me in the right direction? I've been through the REST API Handbook, and it all looks like it should work...

Copy link
Member

Choose a reason for hiding this comment

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

Are you running WordPress 4.8-beta2? This is a new endpoint in 4.8.

credentials: 'include',
} ).then(
( response ) => {
if ( this.unmounting ) {
return;
}
response.json().then( ( obj ) => {
const { html, type } = obj;
if ( html ) {
this.setState( { html, type } );
Copy link
Contributor

Choose a reason for hiding this comment

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

When we do async calls in React components, we need to ensure that the callbacks are not executed if the component has been unmounted before the end of the async process. Which generally means one of these two things:

  • If the promise (or the async process) is abortable, we need to abort it in componentWillUnmount
  • If it's not abortable, we may use a boolean in the component instance this.mounted and do something like if ( this.mounted) return; at the beginning of the async callback.

(Aborting is better when possible)

Copy link
Member Author

Choose a reason for hiding this comment

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

Gah, sorry, I remember you saying this before and I forgot to implement. Will get this done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, fetch isn't abortable, so I've gone with the if ( ! this.mounted ) ... approach

} else {
this.setState( { error: true } );
}
this.setState( { fetching: false } );
} );
}
);
}

return (
<figure className="blocks-embed">
<div className="iframe-overlay">
<iframe src={ url } title={ title } />
</div>
{ ( caption && caption.length > 0 ) || !! focus ? (
<Editable
tagName="figcaption"
placeholder={ wp.i18n.__( 'Write caption…' ) }
value={ caption }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { caption: value } ) }
inline
inlineToolbar
/>
) : null }
</figure>
);
render() {
const { html, type, error, fetching } = this.state;
const { url, caption } = this.props.attributes;
const { setAttributes, focus, setFocus } = this.props;

if ( ! html ) {
return (
<Placeholder icon="cloud" label={ wp.i18n.__( 'Embed URL' ) } className="blocks-embed">
<form onSubmit={ this.doServerSideRender }>
<input
type="url"
className="components-placeholder__input"
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using components-placeholder instead of placeholder?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was an @aduth request, if I recall correctly.

Copy link
Member

@mtias mtias Jun 1, 2017

Choose a reason for hiding this comment

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

Yes, we are using components as prefix for the whole folder. This is so specific to blocks, though. Mmm.

Copy link
Member

Choose a reason for hiding this comment

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

If we are to adhere to the proposed guideline, this should be blocks- prefix, not components-

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

Copy link
Member

Choose a reason for hiding this comment

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

If it's truly generic to be considered a component, it should be moved to the components directory to reflect as such.

Copy link
Member

Choose a reason for hiding this comment

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

No, I agree this is not generic enough and should be blocks-

placeholder={ wp.i18n.__( 'Enter URL to embed here...' ) }
onChange={ ( event ) => setAttributes( { url: event.target.value } ) } />
{ ! fetching
? <Button
isLarge
type="submit">
{ wp.i18n.__( 'Embed' ) }
</Button>
: <Spinner />
}
{ error && <p className="components-placeholder__error">{ wp.i18n.__( 'Sorry, we could not embed that content.' ) }</p> }
</form>
</Placeholder>
);
}

const domain = url.split( '/' )[ 2 ].replace( /^www\./, '' );
Copy link
Member

Choose a reason for hiding this comment

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

This feels a bit fragile, since it could easily throw errors if an unexpected url reaches it (one without at least two slashes). We've already started using Node's url module elsewhere, which can perform much more reliable parsing, specifically via url.parse. Do we need the host 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.

yeah, the host without www. because we want to blacklist certain embeds from previewing in the editor (facebook specifically). I'll look at the url module :)

const cannotPreview = this.noPreview.includes( domain );
Copy link
Member

Choose a reason for hiding this comment

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

Array#includes is not polyfilled (yet) and this will error in IE11.

let typeClassName = 'blocks-embed';

if ( 'video' === type ) {
typeClassName = 'blocks-embed-video';
}

return (
<figure className={ typeClassName }>
{ ( cannotPreview ) ? (
<Placeholder icon="cloud" label={ wp.i18n.__( 'Embed URL' ) }>
<p className="components-placeholder__error"><a href={ url }>{ url }</a></p>
<p className="components-placeholder__error">{ wp.i18n.__( 'Previews for this are unavailable in the editor, sorry!' ) }</p>
</Placeholder>
) : (
<HtmlEmbed html={ html } />
) }
{ ( caption && caption.length > 0 ) || !! focus ? (
<Editable
tagName="figcaption"
placeholder={ wp.i18n.__( 'Write caption…' ) }
value={ caption }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { caption: value } ) }
inline
inlineToolbar
/>
) : null }
</figure>
);
}
},

save( { attributes } ) {
const { url, title, caption } = attributes;
const iframe = <iframe src={ url } title={ title } />;

const { url, caption } = attributes;
if ( ! caption || ! caption.length ) {
return iframe;
return url;
}

return (
<figure>
{ iframe }
{ url }
<figcaption>{ caption }</figcaption>
</figure>
);
Expand Down
8 changes: 4 additions & 4 deletions blocks/library/embed/style.scss
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
.blocks-embed {
.blocks-embed,
.blocks-embed-video {
margin: 0;
}

.blocks-embed .iframe-overlay {
.blocks-embed-video > div:first-child {
position: relative;
width: 100%;
height: 0;
padding-bottom: 56.25%; /* 16:9 */
}

.blocks-embed iframe {
.blocks-embed-video > div > iframe {
position: absolute;
top: 0;
left: 0;
Expand Down Expand Up @@ -55,6 +56,5 @@ div[data-type="core/embed"] {
margin-left: auto;
margin-right: auto;
}

}
}
4 changes: 2 additions & 2 deletions blocks/test/fixtures/core-embed-youtube-caption.html
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:core/embed url="https://www.youtube.com/watch?v=Nl6U7UotA-M" -->
<figure><iframe src="//www.youtube.com/embed/Nl6U7UotA-M" frameborder="0" allowfullscreen></iframe><figcaption>State of the Word 2016</figcaption></figure>
<!-- /wp:core/embed -->
<figure>https://www.youtube.com/embed/Nl6U7UotA-M"<figcaption>State of the Word 2016</figcaption></figure>
<!-- /wp:core/embed -->
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core-embed-youtube-caption.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"uid": "_uid_0",
"blockType": "core/embed",
"attributes": {
"url": "//www.youtube.com/embed/Nl6U7UotA-M",
"url": "https://www.youtube.com/watch?v=Nl6U7UotA-M",
"caption": [
"State of the Word 2016"
]
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!-- wp:core/embed -->
<figure><iframe src="//www.youtube.com/embed/Nl6U7UotA-M"></iframe>
<!-- wp:core/embed url="https://www.youtube.com/watch?v=Nl6U7UotA-M" -->
<figure>https://www.youtube.com/watch?v=Nl6U7UotA-M
<figcaption>State of the Word 2016</figcaption>
</figure>
<!-- /wp:core/embed -->
Expand Down
34 changes: 34 additions & 0 deletions components/html-embed/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/***
* When embedding HTML from the WP oEmbed proxy, we need to insert it
* into a div and make sure any scripts get run. This component takes
* HTML and puts it into a div element, and creates and adds new script
* elements so all scripts get run as expected.
*/
export default class HtmlEmbed extends wp.element.Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand what this component is doing, could you clarify a bit? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, components need to be exported in components/index.js

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, some comments would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment added explaining the purpose of the component

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. Multiline comments (unless jsdoc) should use //.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed!


componentDidMount() {
const body = this.node;
const { html = '' } = this.props;

body.innerHTML = html;

const scripts = body.getElementsByTagName( 'script' );
const newScripts = Array.from( scripts ).map( ( script ) => {
const newScript = document.createElement( 'script' );
if ( script.src ) {
newScript.src = script.src;
} else {
newScript.innerHTML = script.innerHTML;
}
return newScript;
} );

newScripts.forEach( ( script ) => body.appendChild( script ) );
}

render() {
return (
<div ref={ ( node ) => this.node = node } />
);
}
}
1 change: 1 addition & 0 deletions components/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export { default as Button } from './button';
export { default as Dashicon } from './dashicon';
export { default as FormToggle } from './form-toggle';
export { default as HtmlEmbed } from './html-embed';
export { default as IconButton } from './icon-button';
export { default as Panel } from './panel';
export { default as PanelHeader } from './panel/header';
Expand Down
3 changes: 2 additions & 1 deletion components/placeholder/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@
}
}

.components-placeholder__fieldset {
.components-placeholder__fieldset,
.components-placeholder__fieldset form {
display: flex;
flex-direction: row;
justify-content: center;
Expand Down
2 changes: 1 addition & 1 deletion post-content.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ window._wpGutenbergPost = {
'<!-- /wp:core/text -->',

'<!-- wp:core/embed url="https://www.youtube.com/watch?v=Nl6U7UotA-M" -->',
'<figure><iframe src="//www.youtube.com/embed/Nl6U7UotA-M" frameborder="0" allowfullscreen></iframe><figcaption>State of the Word 2016</figcaption></figure>',
'<figure>https://www.youtube.com/watch?v=Nl6U7UotA-M<figcaption>State of the Word 2016</figcaption></figure>',
'<!-- /wp:core/embed -->',

'<!-- wp:core/heading -->',
Expand Down