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

Inner blocks: try hook approach #25633

Merged
merged 1 commit into from
Oct 8, 2020
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Sep 24, 2020

Description

New experimental API.

Currently, <InnerBlocks /> renders a wrapper, but <InnerBlocks.Content> doesn't. Instead, we have to allow customising the wrapper tag name props through the __experimentalTagName and __experimentalPassedProps props.

Currently:

const blockProps = useBlockProps( { className: 'pull' } );

<InnerBlocks
	orientation="vertical"
	__experimentalTagName="blockquote"
	__experimentalPassedProps={ blockWrapperProps }
/>

With a hook:

const props = __experimentalUseInnerBlocksProps(
	useBlockProps( { className: 'pull' } ),
	{ orientation: 'vertical' } // Inner blocks options.
);

<blockquote { ...props } />

Allowing the block author to render the block wrapper gives some more freedom.
You can now render something before or after the content:

const props = __experimentalUseInnerBlocksProps(
	useBlockProps( { className: 'pull' } ),
	{ orientation: 'vertical' } // Inner blocks options.
);

<blockquote { ...props }>
	{ props.children  }
	<cite>{ attributes.cite }</cite>
</blockquote>

In the future, we could allow more control over the children:

const props = __experimentalUseInnerBlocksProps(
	{ className: 'menu' },
	{ orientation: 'vertical' } // Inner blocks options.
);

<ul { ...props }>
	{ props.children.map( ( child ) => <li>{ child }</li> )  }
</ul>

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.

@ellatrix ellatrix added the [Feature] Block API API that allows to express the block paradigm. label Sep 24, 2020
@github-actions
Copy link

github-actions bot commented Sep 24, 2020

Size Change: +57 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-editor/index.js 129 kB +40 B (0%)
build/block-library/index.js 144 kB +17 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 668 B 0 B
build/block-directory/index.js 8.56 kB 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.65 kB 0 B
build/block-library/editor.css 8.65 kB 0 B
build/block-library/style-rtl.css 7.66 kB 0 B
build/block-library/style.css 7.65 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.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.5 kB 0 B
build/components/index.js 169 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.4 kB 0 B
build/compose/index.js 9.43 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.6 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 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.6 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.27 kB 0 B
build/edit-site/index.js 21 kB 0 B
build/edit-site/style-rtl.css 3.73 kB 0 B
build/edit-site/style.css 3.73 kB 0 B
build/edit-widgets/index.js 21.2 kB 0 B
build/edit-widgets/style-rtl.css 3.02 kB 0 B
build/edit-widgets/style.css 3.02 kB 0 B
build/editor/editor-styles-rtl.css 492 B 0 B
build/editor/editor-styles.css 493 B 0 B
build/editor/index.js 45.4 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 734 B 0 B
build/format-library/index.js 7.49 kB 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 621 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 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.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 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.74 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@ZebulanStanphill ZebulanStanphill added [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] New API New API to be used by plugin developers or package users. labels Sep 25, 2020
@ZebulanStanphill
Copy link
Member

Shouldn't it be useInnerBlocksWrapperProps (note the "Blocks") rather than useInnerBlockWrapperProps? Actually, maybe it could just be useInnerBlocksProps?

@ellatrix
Copy link
Member Author

Yeah, definitely with -s, typo :)

@ellatrix
Copy link
Member Author

Actually, maybe it could just be useInnerBlocksProps?

Yeah, that I think that's better. That's why I also preferred useBlockProps, because while you're spreading it on the wrapper, it's really about defining the block itself (which includes the wrapper, being part of the block). @youknowriad What do you think?

@youknowriad
Copy link
Contributor

youknowriad commented Sep 25, 2020

@ellatrix It makes sense to me. One thing though about the "block wrapper" hook, is how to "validate" that it's actually applied to the wrapper element of the block or if there's any way to do it.

@ellatrix
Copy link
Member Author

Do you mean checking if it's attached to an element at all, or checking if it's attached to the outer element.
In the first case, there will be an error if you don't pass the props correctly to an element.
In the second case, a block could be anything, it should be possible to wrap the block with another element that is not the block boundary (as we've seen with alignment wrappers, though this still need to be done better).

@youknowriad
Copy link
Contributor

checking if it's attached to the outer element.

Aside some "framework provided features" (like the alignment maybe), I think the block wrapper should always be on the outer element, there's a lot of assumptions in the code of the block editor that is based on that, we can't render anything there.

@ellatrix
Copy link
Member Author

We could check in the DOM if the parent node is a block list.

@ellatrix ellatrix force-pushed the try/use-inner-block-wrapper-props branch 2 times, most recently from 17a95f9 to db336ab Compare September 25, 2020 18:28
@ellatrix ellatrix requested a review from mkaz as a code owner September 25, 2020 21:47
@ellatrix
Copy link
Member Author

Just 1 e2e test case failure in writing flow with columns... not sure what the cause is.

@ZebulanStanphill
Copy link
Member

@ellatrix In the failing test, there's a part where you're in Navigation mode and you press down to navigate from one Column to the next, and then right to get to a nested Paragraph. However, if you press right too quickly, you stay on the Column. I've also noticed that, for some reason, moving from the first Column to the second seems to take a bit longer than moving from the second Column to its nested Paragraph.

As it turns out, in the same file, there's a comment inside the addParagraphsAndColumnsDemo function that mentions how in some cases, moving from one block to another doesn't happen quickly enough. I guess the best way to workaround it right now would be to throw in some page.waitForSelectors and page.waitForXPaths.

@ellatrix
Copy link
Member Author

Strange that it's failing for this particular branch though.

@ZebulanStanphill
Copy link
Member

@ellatrix I guess it implies this branch has introduced a performance hit somewhere that is causing the test to start failing? Not really sure.

Either way, it's kind of weird that keyboard input is effectively ignored if you do it too quickly. That sounds like a bug that should be fixed.

@ellatrix
Copy link
Member Author

Yeah it works in slowmo...

@ellatrix
Copy link
Member Author

Hm, and sometimes it passes locally, sometimes not. :)

@ellatrix
Copy link
Member Author

Failed at finding the cause today. I'll resume looking tomorrow.

@ellatrix ellatrix force-pushed the try/use-inner-block-wrapper-props branch from 0fc8d06 to fa59374 Compare October 6, 2020 13:58
@ellatrix
Copy link
Member Author

ellatrix commented Oct 6, 2020

I can't figure out why these two tests are not passing.

@ellatrix
Copy link
Member Author

ellatrix commented Oct 7, 2020

OMG! I'm pretty sure it's AsyncModeProvider 😄 It's not wrapped in it...

@ellatrix
Copy link
Member Author

ellatrix commented Oct 7, 2020

Finally, it passes 🎉

@ellatrix ellatrix force-pushed the try/use-inner-block-wrapper-props branch 4 times, most recently from 36b9c27 to 672c67c Compare October 8, 2020 08:03
@ellatrix ellatrix added the [Type] Experimental Experimental feature or API. label Oct 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Type] Experimental Experimental feature or API. [Type] New API New API to be used by plugin developers or package users.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants