Refactor MediaPlaceholder to function component #23671

ntsekouras · 2020-07-03T08:33:21Z

Description

related to #22890

How has this been tested?

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

github-actions · 2020-07-03T08:39:15Z

Size Change: +971 B (0%)

Total Size: 1.13 MB

Filename	Size	Change
`build/api-fetch/index.js`	3.4 kB	-1 B
`build/block-directory/index.js`	7.67 kB	+197 B (2%)
`build/block-editor/index.js`	110 kB	+661 B (0%)
`build/block-editor/style-rtl.css`	10.7 kB	+7 B (0%)
`build/block-editor/style.css`	10.7 kB	+7 B (0%)
`build/block-library/index.js`	130 kB	-33 B (0%)
`build/block-serialization-spec-parser/index.js`	3.1 kB	-5 B (0%)
`build/blocks/index.js`	48.2 kB	+73 B (0%)
`build/components/index.js`	199 kB	+61 B (0%)
`build/compose/index.js`	9.56 kB	-88 B (0%)
`build/core-data/index.js`	11.4 kB	+1 B
`build/data/index.js`	8.45 kB	+1 B
`build/date/index.js`	5.38 kB	-89 B (1%)
`build/edit-navigation/index.js`	9.92 kB	-57 B (0%)
`build/edit-post/index.js`	304 kB	+136 B (0%)
`build/edit-site/index.js`	16.7 kB	+21 B (0%)
`build/edit-widgets/index.js`	9.35 kB	+24 B (0%)
`build/editor/index.js`	45 kB	+192 B (0%)
`build/editor/style-rtl.css`	3.77 kB	-4 B (0%)
`build/editor/style.css`	3.77 kB	-3 B (0%)
`build/format-library/index.js`	7.73 kB	+1 B
`build/media-utils/index.js`	5.32 kB	+23 B (0%)
`build/primitives/index.js`	1.41 kB	-92 B (6%)	✅
`build/rich-text/index.js`	13.9 kB	-99 B (0%)
`build/server-side-render/index.js`	2.71 kB	+36 B (1%)
`build/token-list/index.js`	1.28 kB	+1 B

ℹ️ View Unchanged

Filename	Size	Change
`build/a11y/index.js`	1.14 kB	0 B
`build/annotations/index.js`	3.68 kB	0 B
`build/autop/index.js`	2.82 kB	0 B
`build/blob/index.js`	620 B	0 B
`build/block-directory/style-rtl.css`	944 B	0 B
`build/block-directory/style.css`	945 B	0 B
`build/block-library/editor-rtl.css`	7.57 kB	0 B
`build/block-library/editor.css`	7.57 kB	0 B
`build/block-library/style-rtl.css`	7.78 kB	0 B
`build/block-library/style.css`	7.79 kB	0 B
`build/block-library/theme-rtl.css`	728 B	0 B
`build/block-library/theme.css`	729 B	0 B
`build/block-serialization-default-parser/index.js`	1.88 kB	0 B
`build/components/style-rtl.css`	15.8 kB	0 B
`build/components/style.css`	15.8 kB	0 B
`build/data-controls/index.js`	1.29 kB	0 B
`build/deprecated/index.js`	772 B	0 B
`build/dom-ready/index.js`	569 B	0 B
`build/dom/index.js`	3.19 kB	0 B
`build/edit-navigation/style-rtl.css`	1.02 kB	0 B
`build/edit-navigation/style.css`	1.02 kB	0 B
`build/edit-post/style-rtl.css`	5.57 kB	0 B
`build/edit-post/style.css`	5.57 kB	0 B
`build/edit-site/style-rtl.css`	3.03 kB	0 B
`build/edit-site/style.css`	3.03 kB	0 B
`build/edit-widgets/style-rtl.css`	2.45 kB	0 B
`build/edit-widgets/style.css`	2.45 kB	0 B
`build/editor/editor-styles-rtl.css`	537 B	0 B
`build/editor/editor-styles.css`	539 B	0 B
`build/element/index.js`	4.65 kB	0 B
`build/escape-html/index.js`	733 B	0 B
`build/format-library/style-rtl.css`	547 B	0 B
`build/format-library/style.css`	548 B	0 B
`build/hooks/index.js`	2.13 kB	0 B
`build/html-entities/index.js`	622 B	0 B
`build/i18n/index.js`	3.56 kB	0 B
`build/is-shallow-equal/index.js`	710 B	0 B
`build/keyboard-shortcuts/index.js`	2.52 kB	0 B
`build/keycodes/index.js`	1.94 kB	0 B
`build/list-reusable-blocks/index.js`	3.12 kB	0 B
`build/list-reusable-blocks/style-rtl.css`	476 B	0 B
`build/list-reusable-blocks/style.css`	476 B	0 B
`build/notices/index.js`	1.79 kB	0 B
`build/nux/index.js`	3.41 kB	0 B
`build/nux/style-rtl.css`	671 B	0 B
`build/nux/style.css`	668 B	0 B
`build/plugins/index.js`	2.56 kB	0 B
`build/priority-queue/index.js`	788 B	0 B
`build/redux-routine/index.js`	2.85 kB	0 B
`build/shortcode/index.js`	1.69 kB	0 B
`build/url/index.js`	4.06 kB	0 B
`build/viewport/index.js`	1.85 kB	0 B
`build/warning/index.js`	1.14 kB	0 B
`build/wordcount/index.js`	1.17 kB	0 B

_{compressed-size-action}

packages/block-editor/src/components/media-placeholder/index.js

packages/block-editor/src/components/media-upload/check.js

ZebulanStanphill

Haven't actually tested the branch yet, but the code looks good. Just some minor stylistic nitpicks.

packages/block-editor/src/components/media-placeholder/index.js

ZebulanStanphill · 2020-07-03T15:50:19Z

And of course, right when I post that, I notice there's a failing end-to-end test. 😆 The test seems unrelated, but the failing test does insert an Image block, so perhaps we've both overlooked something.

FAIL packages/e2e-tests/specs/editor/various/block-deletion.test.js (36.931s)
  ● deleting all blocks › gracefully removes blocks when the default block is not available

    expect(received).toBe(expected) // Object.is equality

    Expected: 0
    Received: 1

      201 | 		} );
      202 | 
    > 203 | 		expect( selectedBlocksCount ).toBe( 0 );
          | 		                              ^
      204 | 	} );
      205 | } );
      206 | 

      at Object.<anonymous> (specs/editor/various/block-deletion.test.js:203:33)
          at runMicrotasks (<anonymous>)

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

ItsJonQ · 2020-07-06T14:00:35Z

@ntsekouras Hmm! So strange. I ran the tests locally.

It fails, just like in CI.
However, if I add --puppeteer-interactive --puppeteer-slowmo=200, it passes

packages/block-editor/src/components/media-placeholder/index.js

mcsf

Barring test failure, LGTM!

mcsf · 2020-07-06T14:06:02Z

packages/block-editor/src/components/media-placeholder/index.js

+	const onlyAllowsImages = () =>
+		allowedTypes?.every(
+			( allowedType ) =>
+				allowedType === 'image' || allowedType.startsWith( 'image/' )


String#startsWith isn't implemented in Internet Explorer. I looked at wp-polyfill and the static method does appear to be polyfilled in WordPress trunk as of today (r48333):

Source for locally built wp-polyfill.js

That said, I recommend testing this feature in IE.

packages/block-editor/src/components/media-placeholder/index.js

ntsekouras · 2020-07-07T06:18:34Z

Regarding the failing test here, I added a puppeteer wait for the render to happen properly and I will merge this PR, as it's root failure is not caused by MediaPlaceholder 's refactoring.

Here is the useLayoutEffect React documentation.

Using useLayoutEffect instead of useEffect doesn't fix the test and doesn't make sense to me for these reasons:

The MediaPlaceholder's useEffect handles value property that isn't taken into account for the render
Since we are talking about e2e tests we should wait for the render
Even if it worked we should consider about changing, by checking if it really needed to block the visual updates. We shouldn't just make such changes to any component for e2e tests to pass.

Prefer the standard useEffect when possible to avoid blocking visual updates.

You can easily reproduce the failure with another function component, such as Quote. So the root cause is something related to puppeteer and function components.

If I find the root cause, I'll post an update here and it might help us understand about other failing e2e tests, that seem to fail lately. They could be related, as more and more refactors to function components happen lately too.

ntsekouras · 2020-07-07T14:54:56Z

Hey @ZebulanStanphill! When you find some time take look at it again, as is now blocked from your requested changes.

Thanks!

ZebulanStanphill

Approving since the test failure seems to be unrelated to the refactor. I'm hoping the test issue can be resolved in a less case-specific way in the future, but for now I think the current workaround is acceptable.

ntsekouras requested review from ellatrix and youknowriad as code owners July 3, 2020 08:33

ntsekouras self-assigned this Jul 3, 2020

ntsekouras added [Feature] Media Anything that impacts the experience of managing media [Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality labels Jul 3, 2020

ntsekouras marked this pull request as draft July 3, 2020 10:04

ntsekouras force-pushed the refactor/media-placeholder branch from 0112aa4 to 15685ba Compare July 3, 2020 11:13

ntsekouras marked this pull request as ready for review July 3, 2020 11:13

ntsekouras mentioned this pull request Jul 3, 2020

Code Quality: Refactor all React class components to functional components using hooks #22890

Open

ntsekouras requested a review from mcsf July 3, 2020 11:27

ZebulanStanphill requested changes Jul 3, 2020

View reviewed changes

packages/block-editor/src/components/media-placeholder/index.js Outdated Show resolved Hide resolved

packages/block-editor/src/components/media-upload/check.js Outdated Show resolved Hide resolved

ZebulanStanphill self-requested a review July 3, 2020 15:21

ZebulanStanphill reviewed Jul 3, 2020

View reviewed changes

ntsekouras and others added 9 commits July 6, 2020 10:24

refactor MediaPlaceholder

34bb30d

fix test

2da1fc9

refactor MediaUploadCheck

b69c028

fix multiple upload

dcdad00

Fix dependency in useSelect

c48f7e5

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

Fix dependency in useSelect

4d2841f

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

Use optional chaining

c460101

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

minor refactor

707a8e9

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

minor refactor

f81038a

Co-authored-by: Zebulan Stanphill <zebulanstanphill@protonmail.com>

ntsekouras force-pushed the refactor/media-placeholder branch from 772f97d to f81038a Compare July 6, 2020 07:25

ZebulanStanphill requested changes Jul 6, 2020

View reviewed changes

packages/block-editor/src/components/media-placeholder/index.js Show resolved Hide resolved

mcsf approved these changes Jul 6, 2020

View reviewed changes

ZebulanStanphill reviewed Jul 6, 2020

View reviewed changes

packages/block-editor/src/components/media-placeholder/index.js Outdated Show resolved Hide resolved

ntsekouras added 2 commits July 6, 2020 19:40

minor refactor

e7ad88c

temp e2e fix test

96feeff

ntsekouras requested review from ajitbohra, nerrad, ntwb and talldan as code owners July 6, 2020 17:54

ntsekouras requested a review from ZebulanStanphill July 7, 2020 05:50

ZebulanStanphill approved these changes Jul 7, 2020

View reviewed changes

ntsekouras merged commit 5891e47 into master Jul 7, 2020

ntsekouras deleted the refactor/media-placeholder branch July 7, 2020 15:22

github-actions bot added this to the Gutenberg 8.6 milestone Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor MediaPlaceholder to function component #23671

Refactor MediaPlaceholder to function component #23671

ntsekouras commented Jul 3, 2020

github-actions bot commented Jul 3, 2020 •

edited

Loading

ZebulanStanphill left a comment

ZebulanStanphill commented Jul 3, 2020

ItsJonQ commented Jul 6, 2020

mcsf left a comment

mcsf Jul 6, 2020

ntsekouras commented Jul 7, 2020

ntsekouras commented Jul 7, 2020

ZebulanStanphill left a comment

Refactor MediaPlaceholder to function component #23671

Refactor MediaPlaceholder to function component #23671

Conversation

ntsekouras commented Jul 3, 2020

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

github-actions bot commented Jul 3, 2020 • edited Loading

ZebulanStanphill left a comment

Choose a reason for hiding this comment

ZebulanStanphill commented Jul 3, 2020

ItsJonQ commented Jul 6, 2020

mcsf left a comment

Choose a reason for hiding this comment

mcsf Jul 6, 2020

Choose a reason for hiding this comment

ntsekouras commented Jul 7, 2020

ntsekouras commented Jul 7, 2020

ZebulanStanphill left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 3, 2020 •

edited

Loading