Skip to content

Refactor the inserter menu component and split into multiple smaller components#20880

Merged
youknowriad merged 4 commits intomasterfrom
refactor/inserter
Mar 17, 2020
Merged

Refactor the inserter menu component and split into multiple smaller components#20880
youknowriad merged 4 commits intomasterfrom
refactor/inserter

Conversation

@youknowriad
Copy link
Copy Markdown
Contributor

@youknowriad youknowriad commented Mar 13, 2020

To prepare the addition of the block patterns into the Main Block Inserter, this PR refactors the code base of the InserterMenu component and split it into separate smaller components.
It rewrites some of it with React hooks at the same time.

Testing instructions

  • Try using the inserter, it should work as usual. No behavior change.

@youknowriad youknowriad added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Code Quality Issues or PRs that relate to code quality labels Mar 13, 2020
@youknowriad youknowriad requested a review from ellatrix as a code owner March 13, 2020 14:55
@youknowriad youknowriad self-assigned this Mar 13, 2020
@youknowriad youknowriad requested a review from a team March 13, 2020 14:55
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 13, 2020 •
edited
Loading

Size Change: -82 B (0%)

Total Size: 857 kB

Filename Size Change
build/block-editor/index.js 100 kB -67 B (0%)
build/components/index.js 191 kB -12 B (0%)
build/editor/index.js 44 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 998 B 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 7.23 kB 0 B
build/block-library/editor.css 7.24 kB 0 B
build/block-library/index.js 111 kB 0 B
build/block-library/style-rtl.css 7.42 kB 0 B
build/block-library/style.css 7.43 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/style-rtl.css 15.7 kB 0 B
build/components/style.css 15.7 kB 0 B
build/compose/index.js 6.21 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.2 kB 0 B
build/date/index.js 5.37 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 91.2 kB 0 B
build/edit-post/style-rtl.css 8.52 kB 0 B
build/edit-post/style.css 8.51 kB 0 B
build/edit-site/index.js 5.07 kB 0 B
build/edit-site/style-rtl.css 2.53 kB 0 B
build/edit-site/style.css 2.53 kB 0 B
build/edit-widgets/index.js 4.43 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/style-rtl.css 3.97 kB 0 B
build/editor/style.css 3.96 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.09 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 1.93 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.69 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 4.84 kB 0 B
build/notices/index.js 1.58 kB 0 B
build/nux/index.js 3.01 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@jasmussen
Copy link
Copy Markdown
Contributor

jasmussen commented Mar 16, 2020

Inserter seems to work well for me overall.

One change, though, I see all categories expanded, except "Most Recent". Honestly I think the categories are a failed experiment, so that aspect does not bother me — though if this is an intentional change I'd also make the Most Recent be expanded by default.

gziolo
gziolo reviewed Mar 16, 2020
View reviewed changes
packages/block-editor/src/components/inserter/search-form.js
import { useInstanceId } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

function InserterSearchForm( { onChange } ) {
Copy link
Copy Markdown
Member

@gziolo gziolo Mar 16, 2020

Choose a reason for hiding this comment

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

Technically speaking, it's not a form :)

I'm wondering if it should be converted to form separately.

@youknowriad
Copy link
Copy Markdown
Contributor Author

youknowriad commented Mar 16, 2020

this is an intentional change I'd also make the Most Recent be expanded by default.

@jasmussen

I restored the previous behavior. Let's consider changes here separately (we can maybe remove the collapsing at some point)

@jasmussen
Copy link
Copy Markdown
Contributor

jasmussen commented Mar 16, 2020

Cool, fine with exploring that separately. That was the only behavior change I noted, so per your testing instructions I will defer to Gzregorz on the thumbs up!

gziolo
gziolo reviewed Mar 16, 2020
View reviewed changes
@gziolo
Copy link
Copy Markdown
Member

gziolo commented Mar 16, 2020 •
edited
Loading

There are still some failing unit tests (fetchReusableBlocks might be undefined) and e2e tests.

@youknowriad
Copy link
Copy Markdown
Contributor Author

youknowriad commented Mar 16, 2020

I've been battling with these tests for hours now :). I'm on the right track though :P

@youknowriad
Copy link
Copy Markdown
Contributor Author

youknowriad commented Mar 16, 2020

yay tests fixed.

gziolo
gziolo approved these changes Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Quite an achievement to convert tests to work with React hooks 😃

We need better tools for unit tests and mocking data package in particular.

Code wise this refactoring looks good to go. I haven’t tested.

youknowriad and others added 4 commits March 17, 2020 09:28
@youknowriad youknowriad merged commit 390eac2 into master Mar 17, 2020
@youknowriad youknowriad deleted the refactor/inserter branch March 17, 2020 08:43
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 17, 2020
@StevenDufresne StevenDufresne mentioned this pull request Mar 23, 2020
Closed
@StevenDufresne
Copy link
Copy Markdown
Contributor

StevenDufresne commented Mar 25, 2020

@youknowriad

I believe this PR introduced a recent bug I filed.

#21069

@noisysocks noisysocks mentioned this pull request Jun 18, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gziolo gziolo gziolo approved these changes

@ellatrix ellatrix Awaiting requested review from ellatrix

Assignees

@youknowriad youknowriad

Labels

[Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Milestone

Gutenberg 7.8

Development

Successfully merging this pull request may close these issues.

4 participants

@youknowriad @jasmussen @gziolo @StevenDufresne