-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat: Inserter displays block collections #42405
Conversation
Collections of blocks regsitered via `registerBlockCollection` now appear at the end of the block inserter enabling additional organization of blocks.
Size Change: +937 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
The `BlockTypesList` now expects `sections` to support the `SectionList` used within it for displaying block collections. Prior to this change, an error was thrown.
The `BlockTypesTab` now relies upon additional store selectors. The lack of mocks for these selectors caused test failures.
The code contained redundant selectors against the store for the block type list. This change refactors the native inserter to match the web counterpart, utilizing the selector leveraged by the web.
While verbose, passing explicit props is likely more orthodox and straightforward to interpret.
The key for this component's sections is now `key`, not `id`, which resulted in undefined key values returned here. However, the default `keyExtractor` already searches for `key` and then `id`, so this can be safely removed.
The block created from within this callback appears to be in a format that is incompatible with the native editor at this time. Specifically, this appeared to break Embed block variants, e.g. Vimeo. A future improvement would be to investigate this further in hopes of aligning web and native.
@@ -80,33 +81,63 @@ export default function BlockTypesList( { | |||
listProps.contentContainerStyle | |||
); | |||
|
|||
const renderSection = ( { item } ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While testing these changes, I did not observe performance issues on my devices. However, I explored memoizing renderSection
and its sibling render functions nonetheless. I did not observe much gain in doing so.
It appeared the majority of re-renders are triggered from listProps
changing (listProps
is a large set of attributes from BottomSheet
), which refactoring listProps
is likely outside of the scope of this PR.
That said, I welcome push back on this topic if we feel there are meaningful performance optimization we should put in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this seems outside the scope of this PR. I spent some time measuring performance when adding a block for a separate project this week and believe I also saw listProps
come up a lot as part of that, so seems like something that could be investigated separately 👍
function BlockTypesTab( { onSelect, rootClientId, listProps } ) { | ||
const clipboardBlock = useClipboardBlock( rootClientId ); | ||
|
||
const { blockTypes } = useSelect( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This useSelect
was replaced with useBlockTypesState
to further align the native inserter with the web. I discovered the latter within the web's BlockTypesTab
component, and it appeared to be a hook specifically designed for this use case.
Improve legibility of section headers whenever blocks scroll beneath the header.
Leverage merely an icon with text made dark mode support more challenging. Rendering text allows for easy swapping of color schemes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dcalhoun, thank you for this and for all the helpful notes on the code!
I went through all the test cases you outlined, and experimented with orientation and light/dark mode. All worked perfectly in my testing and I wasn't able to break anything.
The code also reads clearly to me, it's cool that we're closer aligned with the web's implementation following this PR. :)
LGTM 🎉 🚀
@@ -80,33 +81,63 @@ export default function BlockTypesList( { | |||
listProps.contentContainerStyle | |||
); | |||
|
|||
const renderSection = ( { item } ) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this seems outside the scope of this PR. I spent some time measuring performance when adding a block for a separate project this week and believe I also saw listProps
come up a lot as part of that, so seems like something that could be investigated separately 👍
Related PRs
What?
Display block type collections created with
registerBlockCollection
within the native block inserter.Why?
Allow additional organization of block types, similar to the web block inserter.
How?
Leverage
SectionList
in theBlockTypesList
component to allow displaying section headers before each block type collection in the inserter. Given thatSectionList
does not supportnumColumns
, that property is placed upon the nestedFlatList
rendered for each row of blocks.The
BlockTypesTab
andSearchResults
components now structure their block types data in the form of sections. The former now retrieves the collection sections from the store, before populating each section with the relevant block type.Testing Instructions
Self-hosted site displays zero collections
Jetpack-connected site displays a Jetpack collection
Block inserter search functions without error
Reusable blocks tab functions without error
Screenshots or screencast
UX Interactions
block-inserter-collections.MP4
UI Screenshots