From 6aa99448790033f46808a9d8357f712d79d44b97 Mon Sep 17 00:00:00 2001
From: Andrew Duthie <andrew@andrewduthie.com>
Date: Fri, 31 Jan 2020 13:51:49 -0500
Subject: [PATCH] Components: Apply width-based modifier classes to Placeholder
 only when width is known (#19825)

* Components: Apply `is-small` modifier class to Placeholder only when width known

* E2E Tests: Wait for placeholder error display in embed tests

* Testing: Update snapshots for Placeholder class assignment
---
 .../embed/test/__snapshots__/index.js.snap    |  2 +-
 packages/components/src/placeholder/index.js  | 20 ++++++++-----
 .../components/src/placeholder/test/index.js  | 29 +++++++++++++++++--
 .../specs/editor/various/embedding.test.js    |  7 +++++
 storybook/test/__snapshots__/index.js.snap    |  2 +-
 5 files changed, 49 insertions(+), 11 deletions(-)

diff --git a/packages/block-library/src/embed/test/__snapshots__/index.js.snap b/packages/block-library/src/embed/test/__snapshots__/index.js.snap
index eff8cabaaad89..d5267ced08a43 100644
--- a/packages/block-library/src/embed/test/__snapshots__/index.js.snap
+++ b/packages/block-library/src/embed/test/__snapshots__/index.js.snap
@@ -2,7 +2,7 @@
 
 exports[`core/embed block edit matches snapshot 1`] = `
 <div
-  class="components-placeholder is-small wp-block-embed"
+  class="components-placeholder wp-block-embed"
 >
   <iframe
     aria-hidden="true"
diff --git a/packages/components/src/placeholder/index.js b/packages/components/src/placeholder/index.js
index bc960d50dd56a..93905a5d9f5c3 100644
--- a/packages/components/src/placeholder/index.js
+++ b/packages/components/src/placeholder/index.js
@@ -27,13 +27,19 @@ function Placeholder( {
 	...additionalProps
 } ) {
 	const [ resizeListener, { width } ] = useResizeAware();
-	const classes = classnames(
-		'components-placeholder',
-		width >= 320 ? 'is-large' : '',
-		width >= 160 && width < 320 ? 'is-medium' : '',
-		width < 160 ? 'is-small' : '',
-		className
-	);
+
+	// Since `useResizeAware` will report a width of `null` until after the
+	// first render, avoid applying any modifier classes until width is known.
+	let modifierClassNames;
+	if ( typeof width === 'number' ) {
+		modifierClassNames = {
+			'is-large': width >= 320,
+			'is-medium': width >= 160 && width < 320,
+			'is-small': width < 160,
+		};
+	}
+
+	const classes = classnames( 'components-placeholder', className, modifierClassNames );
 	const fieldsetClasses = classnames( 'components-placeholder__fieldset', {
 		'is-column-layout': isColumnLayout,
 	} );
diff --git a/packages/components/src/placeholder/test/index.js b/packages/components/src/placeholder/test/index.js
index 7f8cbc82e5217..d6164d9b496e2 100644
--- a/packages/components/src/placeholder/test/index.js
+++ b/packages/components/src/placeholder/test/index.js
@@ -1,4 +1,3 @@
-jest.mock( 'react-resize-aware' );
 /**
  * External dependencies
  */
@@ -10,8 +9,12 @@ import useResizeAware from 'react-resize-aware';
  */
 import Placeholder from '../';
 
+jest.mock( 'react-resize-aware' );
+
 describe( 'Placeholder', () => {
-	useResizeAware.mockReturnValue( [ <div key="1" />, { width: 320 } ] );
+	beforeEach( () => {
+		useResizeAware.mockReturnValue( [ <div key="1" />, { width: 320 } ] );
+	} );
 
 	describe( 'basic rendering', () => {
 		it( 'should by default render label section and fieldset.', () => {
@@ -77,4 +80,26 @@ describe( 'Placeholder', () => {
 			expect( placeholder.prop( 'test' ) ).toBe( 'test' );
 		} );
 	} );
+
+	describe( 'resize aware', () => {
+		it( 'should not assign modifier class in first-pass `null` width from `useResizeAware`', () => {
+			useResizeAware.mockReturnValue( [ <div key="1" />, { width: 320 } ] );
+
+			const placeholder = shallow( <Placeholder /> );
+
+			expect( placeholder.hasClass( 'is-large' ) ).toBe( true );
+			expect( placeholder.hasClass( 'is-medium' ) ).toBe( false );
+			expect( placeholder.hasClass( 'is-small' ) ).toBe( false );
+		} );
+
+		it( 'should assign modifier class', () => {
+			useResizeAware.mockReturnValue( [ <div key="1" />, { width: null } ] );
+
+			const placeholder = shallow( <Placeholder /> );
+
+			expect( placeholder.hasClass( 'is-large' ) ).toBe( false );
+			expect( placeholder.hasClass( 'is-medium' ) ).toBe( false );
+			expect( placeholder.hasClass( 'is-small' ) ).toBe( false );
+		} );
+	} );
 } );
diff --git a/packages/e2e-tests/specs/editor/various/embedding.test.js b/packages/e2e-tests/specs/editor/various/embedding.test.js
index 7b4215836b983..03f10a227831d 100644
--- a/packages/e2e-tests/specs/editor/various/embedding.test.js
+++ b/packages/e2e-tests/specs/editor/various/embedding.test.js
@@ -210,6 +210,9 @@ describe( 'Embedding content', () => {
 		await page.keyboard.type( 'https://twitter.com/wooyaygutenberg123454312' );
 		await page.keyboard.press( 'Enter' );
 
+		// Wait for the request to fail and present an error.
+		await page.waitForSelector( '.components-placeholder__error' );
+
 		await clickButton( 'Convert to link' );
 		expect( await getEditedPostContent() ).toMatchSnapshot();
 	} );
@@ -232,6 +235,10 @@ describe( 'Embedding content', () => {
 		await page.keyboard.press( 'Enter' );
 		await page.keyboard.type( 'https://twitter.com/wooyaygutenberg123454312' );
 		await page.keyboard.press( 'Enter' );
+
+		// Wait for the request to fail and present an error.
+		await page.waitForSelector( '.components-placeholder__error' );
+
 		// Set up a different mock to make sure that try again actually does make the request again.
 		await setUpResponseMocking( [
 			{
diff --git a/storybook/test/__snapshots__/index.js.snap b/storybook/test/__snapshots__/index.js.snap
index bac08e3d81dfa..0a3e83ad43f5b 100644
--- a/storybook/test/__snapshots__/index.js.snap
+++ b/storybook/test/__snapshots__/index.js.snap
@@ -4062,7 +4062,7 @@ exports[`Storyshots Components/Panel With Icon 1`] = `
 
 exports[`Storyshots Components/Placeholder Default 1`] = `
 <div
-  className="components-placeholder is-small"
+  className="components-placeholder"
 >
   <iframe
     aria-hidden={true}