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

Remove ifBlockEditSelected internal higher-order component #22905

Merged
merged 2 commits into from Jun 5, 2020

Conversation

aduth
Copy link
Member

@aduth aduth commented Jun 4, 2020

This pull request seeks to remove the ifBlockEditSelected internal helper of the @wordpress/block-editor package. The motivation here is (a) to flatten the common block React element hierarchy and (b) to favor hooks-based implementations over higher-order components. ifBlockEditSelected was a useful and expressive helper at the time it was originally implemented, due to the cumbersome nature of using React Context <Consumer> elements. With useContext, it's much simpler now to get direct access to the context object properties relevant for this behavior. For some affected components, it's arguably still more verbose than the previous implementation, but it seems like a reasonable compromise. A flattened element hierarchy may bring some improved performance and debugging benefit.

Testing Instructions:

Verify no regressions in the behavior of affected components. For example, block toolbar and inspector controls should only appear for the selected block.

@aduth aduth added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality [Package] Block editor /packages/block-editor labels Jun 4, 2020
@aduth aduth requested a review from gziolo June 4, 2020 16:01
@aduth aduth added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jun 4, 2020
@github-actions
Copy link

github-actions bot commented Jun 4, 2020

Size Change: +464 B (0%)

Total Size: 1.12 MB

Filename Size Change
build/block-editor/index.js 106 kB +187 B (0%)
build/block-editor/style-rtl.css 11.4 kB +2 B (0%)
build/block-editor/style.css 11.4 kB +2 B (0%)
build/block-library/index.js 126 kB +87 B (0%)
build/block-library/style-rtl.css 7.72 kB +31 B (0%)
build/block-library/style.css 7.72 kB +31 B (0%)
build/components/index.js 193 kB -28 B (0%)
build/components/style-rtl.css 19.5 kB +9 B (0%)
build/components/style.css 19.5 kB +5 B (0%)
build/dom/index.js 3.17 kB +55 B (1%)
build/edit-navigation/style-rtl.css 918 B +40 B (4%)
build/edit-navigation/style.css 919 B +43 B (4%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 3.4 kB 0 B
build/autop/index.js 2.83 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.75 kB 0 B
build/block-directory/style-rtl.css 892 B 0 B
build/block-directory/style.css 892 B 0 B
build/block-library/editor-rtl.css 7.87 kB 0 B
build/block-library/editor.css 7.88 kB 0 B
build/block-library/theme-rtl.css 684 B 0 B
build/block-library/theme.css 686 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.2 kB 0 B
build/compose/index.js 9.31 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.46 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 568 B 0 B
build/edit-navigation/index.js 8.25 kB 0 B
build/edit-post/index.js 302 kB 0 B
build/edit-post/style-rtl.css 5.43 kB 0 B
build/edit-post/style.css 5.43 kB 0 B
build/edit-site/index.js 15 kB 0 B
build/edit-site/style-rtl.css 2.96 kB 0 B
build/edit-site/style.css 2.96 kB 0 B
build/edit-widgets/index.js 8.83 kB 0 B
build/edit-widgets/style-rtl.css 2.4 kB 0 B
build/edit-widgets/style.css 2.4 kB 0 B
build/editor/editor-styles-rtl.css 425 B 0 B
build/editor/editor-styles.css 428 B 0 B
build/editor/index.js 44.7 kB 0 B
build/editor/style-rtl.css 4.26 kB 0 B
build/editor/style.css 4.27 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.72 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 2.13 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 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 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.3 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 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.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.68 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 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

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.

Makes sense, good memories from old times when HOCs were cool :)

@aduth
Copy link
Member Author

aduth commented Jun 4, 2020

The end-to-end tests illustrate a legitimate issue, which is that the previous implementation would pass along props. In this case, it's props.children which is important. It's a fairly straight-forward fix, but there are a couple ways of going about it:

  • Provide all props?
    • return isSelected ? <Fill { ...props } /> : null;
  • Provide children?
    • return isSelected ? <Fill>{ children }</Fill> : null;
  • Always render fill and pass children only if selected?
    • return <Fill>{ isSelected ? children : null }</Fill>;

It could depend on a few things:

  • Is Fill expecting to receive any other props?
  • Would consistently rendering Fill help or hurt performance in any way?

@ellatrix
Copy link
Member

ellatrix commented Jun 5, 2020

I think we should just pass children?

@aduth
Copy link
Member Author

aduth commented Jun 5, 2020

Updated in e6e03c9 to pass only children to the fill (second option from #22905 (comment)). I also confirmed that the implementation of Fill should only require children to be passed. Passing all props could be more thorough, but I think it's best to be explicit about what's expected. This introduces a bit of inconsistency with existing code, which could perhaps be updated separately (this line passing all props).

@gziolo
Copy link
Member

gziolo commented Jun 5, 2020

The only place where props are passed to InsepectorControls:

It doesn't look like there is anything passed down really. It looks not needed.

I agree that using children and nothing else is the most expected approach. isSelected check is correctly placed outside of the Fill – we check whether fills render something to make some decision like if to render HTML wrappers.

@aduth aduth merged commit 7390563 into master Jun 5, 2020
@aduth aduth deleted the remove/if-block-edit-selected branch June 5, 2020 17:01
@github-actions github-actions bot added this to the Gutenberg 8.3 milestone Jun 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Package] Block editor /packages/block-editor [Type] Code Quality Issues or PRs that relate to code quality [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants