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

Port changes from core PR #26417

Merged
merged 4 commits into from
Oct 26, 2020
Merged

Port changes from core PR #26417

merged 4 commits into from
Oct 26, 2020

Conversation

oandregal
Copy link
Member

This PR ports the changes done while preparing the block supports class to be ported to core. See WordPress/wordpress-develop#640

Specifically, this adds the following checks:

  • Only attaches the wrapper if the existing render_callback is a callable (before, only checked for null).
  • Only updates the current block to be rendered if the closure receives a not null WP_Block. This is because there are some dead paths in core that are still used by tests, so we need to cover against that as well. See this comment for context.

How to test

  • Create a new post with the social links block and publish.
  • Go to the front-end and verify that the class wp-block-social-links is attached to the root element (ul) and not the children (li).

@oandregal oandregal self-assigned this Oct 23, 2020
@oandregal oandregal changed the title Port changes from the core PR Port changes from core PR Oct 23, 2020
@oandregal oandregal added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 23, 2020
@github-actions
Copy link

github-actions bot commented Oct 23, 2020

Size Change: +459 B (0%)

Total Size: 1.2 MB

Filename Size Change
build/block-directory/index.js 8.59 kB -1 B
build/block-editor/index.js 130 kB -2 B (0%)
build/block-library/index.js 145 kB -1 B
build/components/index.js 171 kB +62 B (0%)
build/edit-site/index.js 22.1 kB +404 B (1%)
build/format-library/index.js 7.49 kB -3 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.54 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 667 B 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/style-rtl.css 11 kB 0 B
build/block-editor/style.css 11 kB 0 B
build/block-library/editor-rtl.css 8.93 kB 0 B
build/block-library/editor.css 8.93 kB 0 B
build/block-library/style-rtl.css 7.75 kB 0 B
build/block-library/style.css 7.75 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.77 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 kB 0 B
build/components/style-rtl.css 15.4 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.63 kB 0 B
build/core-data/index.js 12.1 kB 0 B
build/data-controls/index.js 684 B 0 B
build/data/index.js 8.63 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 4.43 kB 0 B
build/edit-navigation/index.js 10.8 kB 0 B
build/edit-navigation/style-rtl.css 881 B 0 B
build/edit-navigation/style.css 885 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.37 kB 0 B
build/edit-post/style.css 6.35 kB 0 B
build/edit-site/style-rtl.css 3.79 kB 0 B
build/edit-site/style.css 3.79 kB 0 B
build/edit-widgets/index.js 26.6 kB 0 B
build/edit-widgets/style-rtl.css 3.09 kB 0 B
build/edit-widgets/style.css 3.09 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 42.6 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 kB 0 B
build/element/index.js 4.45 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 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 709 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 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/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 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.45 kB 0 B
build/primitives/index.js 1.35 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/reusable-blocks/index.js 3.06 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.61 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.23 kB 0 B

compressed-size-action

Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Left a note, but LGTM!

WP_Block_Supports::$block_to_render = $parent_block;
return $result;
} else {
return $block_render_callback( $attributes, $content );
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor stylistic point: I'd prefer the early-return method (which we tend to prefer in JS too) over the if-else pair. And that way we also avoid double negatives like !== null:

if ( null === $block ) {
  return $block_render_callback( $attributes, $content );
}

$parent_block = …

Copy link
Member

Choose a reason for hiding this comment

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

Extra advantage for early-return – now the main logic branch is at the top execution level and the error handling is an obvious detail at a deeper level.

P.S. Sorry for the drive-by comment… :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@nb: I still remember Else considered harmful. ;)

@oandregal
Copy link
Member Author

I want to coordinate changes from this PR WordPress/wordpress-develop#640 before merging this one.

@oandregal
Copy link
Member Author

WordPress/wordpress-develop#640 was merged so I'm landing this one as well. Note that there's a failing end-to-end test. This is because CI is using latest wordpress-develop, so it'll be fixed as soon as it picks up the new patched version.

@oandregal oandregal merged commit a4af7d0 into master Oct 26, 2020
@oandregal oandregal deleted the update/block-supports branch October 26, 2020 08:33
@github-actions github-actions bot added this to the Gutenberg 9.3 milestone Oct 26, 2020
oandregal pushed a commit that referenced this pull request Oct 26, 2020
…ss (#26417)

This PR ports the changes from the core PR at WordPress/wordpress-develop#640

Co-authored-by: Jon Surrell <[email protected]>
Co-authored-by: Miguel Fonseca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[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