Block editor: create useBlockRef and useBlockElement for better access #29573
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +669 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Mar 5, 2021
packages/block-editor/src/components/block-list/block-popover.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Mar 5, 2021
packages/block-editor/src/components/block-list/use-block-props/use-block-refs.js
Outdated
Show resolved
Hide resolved
youknowriad
reviewed
Mar 5, 2021
ellatrix
force-pushed
the
try/block-node-access
branch
from
b326f99 to
902c9c6
Compare
ellatrix
commented
Apr 27, 2021
| @@ -235,6 +222,9 @@ function BlockPopover( { | |||
| __experimentalOnIndexChange={ ( index ) => { | |||
| initialToolbarItemIndexRef.current = index; | |||
| } } | |||
| // Resets the index whenever the active block changes so | |||
| // this is not persisted. See https://github.com/WordPress/gutenberg/pull/25760#issuecomment-717906169 | |||
| key={ clientId } | |||
There was a problem hiding this comment.
Remounting the component is better because the block element may change immediately from one to the other. Previously the block node would be undefined between client ID changes.
ellatrix
changed the title
ellatrix
commented
Apr 27, 2021
| @@ -100,8 +96,7 @@ function InsertionPointPopover( { | |||
| 'vertical'; | |||
|
|
|||
| return { | |||
| previousElement: getBlockDOMNode( previous, ownerDocument ), | |||
| nextElement: getBlockDOMNode( next, ownerDocument ), | |||
| previousClientId: previous, | |||
There was a problem hiding this comment.
Note that previously the element wouldn't update if it changed because it's memoized by client ID.
ellatrix
commented
Apr 27, 2021
| * | ||
| * @return {Node|undefined} Preview container DOM node. | ||
| */ | ||
| export function getBlockPreviewContainerDOMNode( clientId, doc ) { |
There was a problem hiding this comment.
This function is not used anywhere.
This was referenced Apr 27, 2021
ellatrix
commented
Apr 27, 2021
| @@ -53,7 +49,7 @@ export default function BlockList( { className, __experimentalLayout } ) { | |||
| }, [] ); | |||
|
|
|||
| return ( | |||
| <BlockNodes.Provider value={ blockNodes }> | |||
| <> | |||
| { insertionPoint } | |||
| <BlockPopover /> | |||
There was a problem hiding this comment.
After this and #31134, we can move the popover and insertion point out of the BlockList component.
youknowriad
reviewed
Apr 27, 2021
| }, [] ); | ||
| return useCallback( ( element ) => { | ||
| // Update the ref in the provider. | ||
| ref.current = element; |
There was a problem hiding this comment.
I wonder if we could just set the element in the context here instead of using an intermediary "ref". I thought the "ref" was useful for useBlockRef, but it doesn't seem to be the case anyway.
There was a problem hiding this comment.
We could through useMergeRefs, but we'd need to callback anyway as well, so this seems a bit simpler.
youknowriad
approved these changes
Apr 27, 2021
aristath
approved these changes
Apr 27, 2021
youknowriad
reviewed
Apr 27, 2021
| @@ -24,7 +25,7 @@ function BlockEditorProvider( props ) { | |||
| // Syncs the entity provider with changes in the block-editor store. | |||
| useBlockSync( props ); | |||
|
|
|||
| return children; | |||
| return <BlockRefsProvider>{ children }</BlockRefsProvider>; | |||
There was a problem hiding this comment.
Since this is now a Provider inside the state provider, it makes me wonder whether we really need this new provider and whether we can just use the store to keep track of these refs.
There was a problem hiding this comment.
Hm, I'm not sure. Then we'd have refs and elements in the redux state?
There was a problem hiding this comment.
Is that an issue? 🤷♂️That doesn't bother me personally, it's not a part of the state that is meant to be serializable
There was a problem hiding this comment.
To clarify, it doesn't bother me as right now, I'm just wondering whether it's just the same thing and we could avoid the extra state/provider
There was a problem hiding this comment.
I can try, but we'd still keep the mutable maps because otherwise we're rerendering the editor too much on load
7 tasks
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #29475.
Fixes #29457.
This is sorely needed to be able to access the a block node by client ID that doesn't rely on querying the document and doesn't need
ownerDocument. Currently, the skip to selected block button and colour panels are broken for the iframed site editor.useBlockRefgets a ref that points to the current block node anduseBlockElementreturn the element directly and updates whenever the element changes.We do currently provide block nodes, but it's limited to the selected block nodes and only works within the
BlockList. This PR also moved the context to theBlockEditorProviderso it can be accessed by all components.How has this been tested?
Screenshots
Types of changes
Checklist: