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.

Sign up for GitHub

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

Jump to bottom

Components: Apply width-based modifier classes to Placeholder only when width is known #19825

Merged
merged 3 commits into from
Jan 31, 2020
Merged

Components: Apply width-based modifier classes to Placeholder only when width is known #19825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7170667
Components: Apply `is-small` modifier class to Placeholder only when …
aduth Jan 22, 2020
01ebb79
E2E Tests: Wait for placeholder error display in embed tests
aduth Jan 22, 2020
97b1fa8
Testing: Update snapshots for Placeholder class assignment
aduth Jan 22, 2020
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
2 changes: 1 addition & 1 deletion packages/block-library/src/embed/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
20 changes: 13 additions & 7 deletions packages/components/src/placeholder/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

Choose a reason for hiding this comment

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

Alternatively you could manually measure the element sizes and apply them as default values.

const [ resizeListener, { width = elementRef.current.clientWidth } ] = useResizeAware();

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @FezVrasta. If I understand correctly, I don't think that would be possible.

  • A ref to a DOM node would only be assigned after the first render. It was my understanding this was also the reason react-resize-aware would provide null initially.
  • The specific syntax wouldn't work, since the defaulting would not take effect if the value of width is null (only if it were undefined).
const sizes = { width: null };
const { width = 'somedefault' } = sizes;
console.log( width );
// null

Copy link

Choose a reason for hiding this comment

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

My bad, you are right, you would need an outer component with its own wrapping markup to do that.

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,
} );
Expand Down
29 changes: 27 additions & 2 deletions packages/components/src/placeholder/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
jest.mock( 'react-resize-aware' );
/**
* External dependencies
*/
Expand All @@ -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.', () => {
Expand Down Expand Up @@ -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 );
} );
} );
} );
7 changes: 7 additions & 0 deletions packages/e2e-tests/specs/editor/various/embedding.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
} );
Expand All @@ -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( [
{
Expand Down
2 changes: 1 addition & 1 deletion storybook/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down