More embed block fixes #1001

notnownikki · 2017-06-02T13:26:42Z

Loading the rendered html on loading a saved block moved into componentWillMount
Removed '.includes' usage, as it's not polyfilled
Added a new rendering state for when we're loading a saved block
Replaced string manipulation of urls to extract the host, with url.parse

* Loading the rendered html on loading a saved block moved into componentWillMount * Removed '.includes' usage, as it's not polyfilled * Added a new rendering state for when we're loading a saved block * Replaced string manipulation of urls to extract the host, with url.parse

mtias · 2017-06-02T13:30:54Z

blocks/library/embed/index.js

 			const { url, caption } = this.props.attributes;
 			const { setAttributes, focus, setFocus } = this.props;

+			if ( loadingFromSavedBlock ) {
+				// we're loading from a saved block, but haven't fetched the HTML yet...
+				return (


@jasmussen what do you think should be the UX here? With this change it'd be the url as text shown until we can load the generated embed. Maybe a white box saying "Generating embed..." with the url in smaller text could be better?

mtias · 2017-06-02T13:31:50Z

blocks/library/embed/index.js

 					} );
 				}
 			);
 		}

 		render() {
-			const { html, type, error, fetching } = this.state;
+			const { html, type, error, fetching, loadingFromSavedBlock } = this.state;


I think ideally this should be the single loading state, and it doesn't matter whether you just input the url in the embed field, or if it's loading from saved content.

I think we need @jasmussen to cast a design spell on the loading state so it's suitable for both purposes 😄

Use the same UI state for fetching new embedded content and loading saved embed blocks in a post

…edding fails

aduth · 2017-06-05T17:51:50Z

blocks/library/embed/index.js

+				return false;
+			}
+			host = host.replace( 'www.', '' );
+			for ( let i = 0; i < this.noPreview.length; i++ ) {


While we don't have native Array#includes, we can still pull in the Lodash variation which dramatically simplifies the function:

isPreviewBlacklisted( host ) { return includes( this.noPreview, host.replace( 'www.', '' ) ); }

Couple questions on the replace:

www is a proper subdomain that can serve different content than the bare host. Is it appropriate in all cases to replace it? Are we expecting both www and non-www Facebook embeds?

If we want to keep www. replacing, probably want to test it at the beginning so that we don't get false positives like isPreviewBlacklisted( 'facewww.book.com' ). This would likely be a regular expression replace, host.replace( /^www\./, '' )

oEmbed allows the user to embed with and without the www, even though facebook puts the www back in the embed html when its scripts run. So we need to account for that here.

aduth · 2017-06-05T18:02:21Z

blocks/library/embed/index.js

@@ -85,8 +91,14 @@ registerBlockType( 'core/embed', {
 			this.noPreview = [
 				'facebook.com',


I couldn't find much background on why we need to disable previews for some hosts. Should this be performed on the server instead of on the client? Also doesn't seem to need to be an instance variable assigned to each occurrence of the component but rather a top-level shared constant reference e.g. PREVIEW_HOST_BLACKLIST.

Eventually we'll move the embedded previews in the editor into sandbox iframes. Facebook embeds refused to run inside these iframes, and throw up errors, so we want to display a "sorry, preview not available" message instead.

Though, yeah, noPreview should be top level and named better. Will fix!

aduth · 2017-06-05T18:03:49Z

blocks/library/embed/index.js

+/**
+ * External dependencies
+ */
+import { parse } from 'url';


Minor: There's a bit of an undocumented (semi-documented?) convention around ordering these blocks External -> WordPress -> Internal (from most external to least external).

aduth · 2017-06-05T18:04:57Z

blocks/library/embed/index.js

@@ -76,6 +81,7 @@ registerBlockType( 'core/embed', {
 		constructor() {
 			super( ...arguments );
 			this.doServerSideRender = this.doServerSideRender.bind( this );
+			this.isPreviewBlacklisted = this.isPreviewBlacklisted.bind( this );


Technically this function may not need to be bound because it's never called in a context that would have a this binding anything other than the current component instance (usually callbacks). But I think it's probably wise to err on the side of safety and bind every component function.

This is getting removed now that the noPreview domains are getting slightly reworked.

aduth · 2017-06-05T18:05:54Z

blocks/library/embed/index.js

+				return (
+					<div className="blocks-embed__loading">
+						<Spinner />
+						<p className="blocks-embed__loading-text">{ wp.i18n.__( 'Embedding...' ) }</p>


Minor: Ellipsis character … is more semantic than three dots for this purpose.

aduth · 2017-06-05T18:07:13Z

blocks/library/embed/index.js

 							{ 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\./, '' );
-			const cannotPreview = this.noPreview.includes( domain );
+			const urlBits = parse( url );


I might recommend a more meaningful name like parsedUrl than "bits" here.

But "bits" is cute! Sure, will change :)

* Elipsis character instead of ... * Removed cuteness from parsed url variable name * Moved "no preview" list of domains into top level const * Used lodash include * Fixed the www. removing regex * Fixed order of imports

aduth

Good to go after modifier class change 👍

aduth · 2017-06-07T13:32:25Z

blocks/library/embed/index.js

@@ -129,31 +140,38 @@ registerBlockType( 'core/embed', {
 			const { url, caption } = this.props.attributes;
 			const { setAttributes, focus, setFocus } = this.props;

+			if ( fetching ) {
+				return (
+					<div className="blocks-embed__loading">


The docs could probably do for some clarification, but it's intended that the root element of the return value should simply be top-level-folder + folder-name, i.e. blocks-embed. The treatment here is one reserved for descendants, or other components in the same folder (not index.js).

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

Would be more appropriate to apply loading as a modifier class:

<div className="blocks-embed is-loading">

mtias reviewed Jun 2, 2017

View reviewed changes

mtias added the [Feature] Blocks Overall functionality of blocks label Jun 2, 2017

notnownikki mentioned this pull request Jun 3, 2017

React warning in Embed block. #1015

Closed

jasmussen and others added 3 commits June 5, 2017 12:05

Add some loading polish.

78f04bf

Single "fetching content" UI

a1d5789

Use the same UI state for fetching new embedded content and loading saved embed blocks in a post

Set the value of the url input so that errors can be corrected if emb…

88f7ef0

…edding fails

aduth reviewed Jun 5, 2017

View reviewed changes

notnownikki added 2 commits June 6, 2017 12:38

More fixes...

d2525c7

* Elipsis character instead of ... * Removed cuteness from parsed url variable name * Moved "no preview" list of domains into top level const * Used lodash include * Fixed the www. removing regex * Fixed order of imports

Removed redundant noPreview includes method

8d35238

aduth requested changes Jun 7, 2017

View reviewed changes

Reorg classes for the embed fetching UI

bfe609d

notnownikki merged commit ef0e193 into master Jun 7, 2017

mtias deleted the update/embed-block-fixes branch June 12, 2017 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More embed block fixes #1001

More embed block fixes #1001

notnownikki commented Jun 2, 2017

mtias Jun 2, 2017

mtias Jun 2, 2017

notnownikki Jun 2, 2017

aduth Jun 5, 2017

notnownikki Jun 6, 2017

aduth Jun 5, 2017

notnownikki Jun 6, 2017

aduth Jun 5, 2017

aduth Jun 5, 2017

notnownikki Jun 6, 2017

aduth Jun 5, 2017

aduth Jun 5, 2017

notnownikki Jun 6, 2017

aduth left a comment

aduth Jun 7, 2017

		@@ -85,8 +91,14 @@ registerBlockType( 'core/embed', {
		this.noPreview = [
		'facebook.com',

More embed block fixes #1001

More embed block fixes #1001

Conversation

notnownikki commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment