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 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: 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 ) {
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, {
credentials: 'include',
} ).then(
( response ) => {
if ( this.unmounting ) {
return;
}
response.json().then( ( obj ) => {
const { html, type } = obj;
if ( html ) {
this.setState( { html, type } );
} 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
33 changes: 33 additions & 0 deletions components/html-embed/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
// 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 {

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