Embed block refactor and tidy #10958

notnownikki · 2018-10-23T15:47:24Z

Description

Refactor to use components, rename the maybeSwitchBlock method, and move class name calculations into a util function.

How has this been tested?

e2e tests pass, new unit tests pass.

Types of changes

Refactor

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…refactor

talldan

❤️ This is looking really tidy! Would be really happy to see this merged as is, though I left a few minor comments.

talldan · 2018-10-31T09:29:18Z

packages/block-library/src/embed/edit.js


 			this.props.setAttributes(
 				{
 					allowResponsive: ! allowResponsive,
-					className: classnames( className, responsiveClassNames ),
+					className: getClassNames( html, className, responsive && ! allowResponsive ),


Just wondering what the responsive variable is, is it whether the type of embed supports responsiveness?

On a separate note, it might be nice to extract ! allowResponsive into a separate variable so that it doesn't have to be repeated.

Yes, responsive is a flag for if the block allows responsive content or not.

talldan · 2018-10-31T09:36:08Z

packages/block-library/src/embed/edit.js

-						<Spinner />
-						<p>{ __( 'Embedding…' ) }</p>
-					</div>
+					<EmbedLoading />


❤️ that these are now separate components, makes the rendering logic so much more understandable.

talldan · 2018-10-31T09:37:48Z

packages/block-library/src/embed/edit.js

@@ -90,7 +90,8 @@ export function getEmbedEditComponent( title, icon, responsive = true ) {
 			previewDocument.body.innerHTML = html;
 			const iframe = previewDocument.body.querySelector( 'iframe' );

-			if ( ! isFromWordPress( html ) && iframe && iframe.height && iframe.width ) {
+			// If we have a fixed aspect iframe, and it's a responsive embed block.
+			if ( responsive && iframe && iframe.height && iframe.width ) {


Looks like the ! isFromWordPress( html ) was lost in the merge, not sure if that's an issue or if it has been replace by the responsive variable.

Replaced by the responsive flag :)

talldan · 2018-10-31T09:43:42Z

packages/block-library/src/embed/components.js

+} from '@wordpress/components';
+import { RichText, BlockControls, BlockIcon, InspectorControls } from '@wordpress/editor';
+
+export const EmbedLoading = () => (


Massive improvement that these are extracted, though I think now they could go in their own files 😄

talldan · 2018-10-31T09:45:44Z

packages/block-library/src/embed/components.js

+	);
+};
+
+export const EmbedEditUrl = ( props ) => {


This could potentially be called EmbedPlaceholder, which would make it consistent with the MediaPlaceholder that's used in the various media blocks.

talldan · 2018-10-31T10:04:37Z

packages/block-library/src/embed/util.js

+ * @return {string} Deduped class names.
+ */
+export function getClassNames( html, existingClassNames = '', allowResponsive = true ) {
+	const previewDocument = document.implementation.createHTMLDocument( '' );


I suppose one future improvement is that this function is quite expensive when allowResponsive is false, when really all it has to do is strip the responsive classnames out of the string.

talldan · 2018-10-31T10:12:59Z

packages/block-library/src/embed/util.js

+/**
+ * Returns class names with any relevant responsive aspect ratio names.
+ *
+ * @param {string} html The preview HTML that possibly contains an iframe with width and height set.


Nitpick, I think the params are supposed to be lined up like they are here:

gutenberg/packages/core-data/src/entities.js

Lines 57 to 66 in 55d5aac

/**

* Returns the entity's getter method name given its kind and name.

*

* @param {string} kind Entity kind.

* @param {string} name Entity name.

* @param {string} prefix Function prefix.

* @param {boolean} usePlural Whether to use the plural form or not.

*

* @return {string} Method name

*/

Although it's actually really hard to find a good example of this, and I'm sure I just created some doc blocks where they weren't lined up myself 🤦‍♂️

notnownikki · 2018-10-31T10:22:11Z

@talldan thanks! Those points look simple enough to address. Will get on it!

notnownikki · 2018-10-31T11:32:42Z

All these should be addressed now. I also renamed some variables in the controls component to make it clear when responsive support was at the theme and block level.

notnownikki · 2018-10-31T15:49:17Z

@talldan could you give this a quick look over before I merge? Just want to make sure I haven't introduced any other problems :)

talldan

@notnownikki Changes look really good 👍

The only minor comment is that It'd be nice to remove the components.js file and import the components directly in edit.js.

talldan · 2018-11-01T09:00:44Z

packages/block-library/src/embed/components.js

-		</Placeholder>
-	);
-};
+export { default as EmbedControls } from './embed-controls';


Don't really need this file anymore, since the components can be imported directly from their own files in embed/edit.js

talldan · 2018-11-01T09:03:31Z

packages/block-library/src/embed/edit.js

@@ -166,7 +167,8 @@ export function getEmbedEditComponent( title, icon, responsive = true ) {
 				<Fragment>
 					<EmbedControls
 						showEditButton={ preview && ! cannotEmbed }
-						supportsResponsive={ supportsResponsive }
+						themeSupportsResponsive={ themeSupportsResponsive }
+						blockSupportsResponsive={ responsive }


I like these new names 👍

talldan · 2018-11-01T09:03:51Z

packages/block-library/src/embed/util.js

 * @return {string} Deduped class names.
 */
 export function getClassNames( html, existingClassNames = '', allowResponsive = true ) {
+	if ( ! allowResponsive ) {


…rnmobile/port-quote-block-step-1 * 'master' of https://github.com/WordPress/gutenberg: (22 commits) Add removed periods to block descriptions. (#11367) Remove findDOMNode usage from the inserter (#11363) Remove deprecated componentWillReceiveProps from TinyMCE component (#11368) Create file blocks when dropping multiple files at once (#11297) Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168) Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180) Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342) Remove the Cloudflare warning (#11350) Image Block: Use source_url for media file link (#11254) Enhance styling of nextpage block using the Hr element (#11354) Embed block refactor and tidy (#10958) Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347) Fix RTL block alignments (#11293) RichText: fix buggy enter/delete behaviour (#11287) Remove code coverage setup (#11198) Parser: Runs all parser implementations against the same tests (#11320) Stop trying to autosave when title and classic block content both are empty. (#10404) Fix "Mac OS" typo + use fancy quotes consistently (#11310) Update documentation link paths (#11324) Editor: Reshape blocks state under own key (#11315) ... # Conflicts: # gutenberg-mobile

notnownikki added 5 commits October 23, 2018 14:54

Edit using separate components for each state

b8e02f4

Change maybe to upgrade

ddf0f98

WordPress embeds do their own iframe thing, don't make them responsive

71a2af3

Embed edit components

ececfe9

Docs explaining why we need attributes passed in separately

0733d41

notnownikki added the [Status] In Progress Tracking issues with work in progress label Oct 23, 2018

notnownikki added 3 commits October 29, 2018 10:42

Merge remote-tracking branch 'origin/master' into update/embed-block-…

f34d9ea

…refactor

Moved class name calculations into a util function

c0eeaf2

Tests for class name calculation

c7fdf5c

notnownikki removed the [Status] In Progress Tracking issues with work in progress label Oct 29, 2018

notnownikki requested review from tofumatt and talldan October 29, 2018 14:50

talldan approved these changes Oct 31, 2018

View reviewed changes

Components in separate files, better variable naming for support

3d87a41

talldan approved these changes Nov 1, 2018

View reviewed changes

Removed components file, removed unneeded editingUrl state change

a6d0d15

notnownikki merged commit 52119c3 into master Nov 1, 2018

talldan deleted the update/embed-block-refactor branch November 2, 2018 03:14

notnownikki mentioned this pull request Nov 2, 2018

Video Block doesn't work when inserting YouTube URL #6824

Closed

notnownikki mentioned this pull request Nov 6, 2018

Facebook Embed Block preview error doesn't differentiate from a oEmbed API error #8361

Closed

mtias added this to the 4.3 milestone Nov 12, 2018

mtias added [Block] Embed Affects the Embed Block [Type] Code Quality Issues or PRs that relate to code quality labels Nov 12, 2018

mashikag mentioned this pull request Apr 7, 2022

Flickr embeds are low quality in the editor Automattic/wp-calypso#60210

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Embed block refactor and tidy #10958

Embed block refactor and tidy #10958

notnownikki commented Oct 23, 2018 •

edited

Loading

talldan left a comment

talldan Oct 31, 2018

notnownikki Oct 31, 2018

talldan Oct 31, 2018

talldan Oct 31, 2018

notnownikki Oct 31, 2018

talldan Oct 31, 2018

talldan Oct 31, 2018

talldan Oct 31, 2018

talldan Oct 31, 2018

talldan Oct 31, 2018

notnownikki commented Oct 31, 2018

notnownikki commented Oct 31, 2018

notnownikki commented Oct 31, 2018

talldan left a comment

talldan Nov 1, 2018

talldan Nov 1, 2018

talldan Nov 1, 2018

	/**
	* Returns the entity's getter method name given its kind and name.
	*
	* @param {string} kind Entity kind.
	* @param {string} name Entity name.
	* @param {string} prefix Function prefix.
	* @param {boolean} usePlural Whether to use the plural form or not.
	*
	* @return {string} Method name
	*/

Embed block refactor and tidy #10958

Embed block refactor and tidy #10958

Conversation

notnownikki commented Oct 23, 2018 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

talldan left a 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

notnownikki commented Oct 31, 2018

notnownikki commented Oct 31, 2018

notnownikki commented Oct 31, 2018

talldan left a 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

notnownikki commented Oct 23, 2018 •

edited

Loading