New useBlockElementRef hook for storing block element into a ref #63799
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
jsnajdr
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -20 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Jul 22, 2024
| */ | ||
| function useBlockRef( clientId ) { | ||
| export function useBlockElementRef( clientId, ref ) { |
There was a problem hiding this comment.
Why can't we set the ref that is provided here directly on refsMap to be used by the block?
There was a problem hiding this comment.
You mean, the refs would be stored directly in the refsMap instead of the element? And the refsMap job would be to "forward" the ref callback calls to the registered consumers?
I think in that case it would be hard to register a new ref at a time when the block is already mounted. When calling:
useBlockElementRef( clientId, ref );The ref wouldn't be set until the block element is changed or recreated.
ellatrix
reviewed
Jul 22, 2024
| ref.current = el; | ||
| } | ||
| }, | ||
| [ ref ] |
There was a problem hiding this comment.
Could we not avoid the useCallback here by moving the function within the useLayoutEffect that follows?
There was a problem hiding this comment.
Or setRef( ref, element ) could even be outside the hook
There was a problem hiding this comment.
Haha, there's actually also assignRef in use-merge-refs
There was a problem hiding this comment.
Not that we can use it here, it's a different package
gutenberg/packages/compose/src/hooks/use-merge-refs/index.js
Lines 18 to 27 in 47bde1a
There was a problem hiding this comment.
Here we need to ensure that when the ref parameter in useBlockElementRef( clientId, ref ) changes, the new version is called immediately. That's what React also does. When a different ref is passed to <div ref={ ref } />, then even though the div element is still the same, it will call the old ref with null and the new ref with the element.
There was a problem hiding this comment.
The layout effect will still be called when adding ref as a dependency?
There was a problem hiding this comment.
Not that we can use it here, it's a different package
Interesting, I didn't know about that. The ref.hasOwnProperty( 'current' ) check is nice, but not even React bothers to do it. It checks ref !== null, typeof ref === 'function', and then if ref.current assignment leads to a crash, it will crash, it's you fault.
There was a problem hiding this comment.
The layout effect will still be called when adding ref as a dependency?
Now I get it, of course you're right, fixing in 26e153a. It's a nice simplification.
ellatrix
reviewed
Jul 22, 2024
packages/block-editor/src/components/block-list/use-block-props/use-block-refs.js
Outdated
Show resolved
Hide resolved
ellatrix
approved these changes
Jul 22, 2024
jsnajdr
force-pushed
the
improve/use-block-refs
branch
from
26e153a to
9f4ee9b
Compare
fabiankaegy
added
the
Needs Dev Note
97 tasks
|
Hey @jsnajdr 👋 Would you be able to help write a dev note for this for the 6.7 release? We are planning to have this as part of a larger Miscellaneous Editor Updates note. We are hoping to get all drafts in by October 13th to leave some time for reviews before the RC1. All Dev Notes get tracked in #65784 so feel free to leave a note there or ping me directly :) Please let us know if you can assist with that. Thanks in advance :) |
|
Hi @fabiankaegy 👋 I believe that dev note is not needed here, because the PR modifies only internal APIs used inside the |
|
Ahh my bad. Will remove it from the list then :) |
fabiankaegy
removed
the
Needs Dev Note
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.
Implements idea from #63565 (comment), replacing the
useBlockRefhook with a newuseBlockElementRefone. The difference is that the hook doesn't return a ref, but accepts one as a parameter. That's similar to how a JSX markup like<div ref={ divRef } />also accepts a ref as a parameter.This hook can be used to easily implement the
useBlockElementhook.This PR also removes the
__unstableprefixes from the exports because they are all internal API anyway, they are not exported from theblock-editorpackage.For correct types, I also had to add exports of the
RefCallbackandRefReact types toelement.