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.

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

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

Merged
merged 4 commits into from
Mar 17, 2020

Conversation

youknowriad
Copy link
Contributor

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

github-actions bot commented Mar 13, 2020

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
Contributor

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.

import { useInstanceId } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

function InserterSearchForm( { onChange } ) {
Copy link
Member

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
Contributor Author

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
Contributor

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
Copy link
Member

gziolo commented Mar 16, 2020

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

@youknowriad
Copy link
Contributor Author

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

@youknowriad
Copy link
Contributor Author

yay tests fixed.

Copy link
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.

@StevenDufresne
Copy link
Contributor

@youknowriad

I believe this PR introduced a recent bug I filed.

#21069

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

4 participants