Block editor: improve root portal #48803
Closed
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
ellatrix
commented
Mar 6, 2023
| @@ -135,7 +183,7 @@ export default function BlockList( settings ) { | |||
| ); | |||
| } | |||
|
|
|||
| BlockList.__unstableElementContext = elementContext; | |||
| BlockList.useRootPortal = useRootPortal; | |||
There was a problem hiding this comment.
Would be good to also make this unstable for now, but eventually this should be exported in the block editor package as an official API.
There was a problem hiding this comment.
I guess we can start private.
|
Size Change: +33 B (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e8f0a0d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4347423639
|
kevin940726
reviewed
Mar 7, 2023
There was a problem hiding this comment.
It will be nice if you could add some API details and expectation in the PR description so that we can learn more about this new hook and how to use it. If the goal is to stablize it as a public API then we would also want to have some unit tests in place too.
I still think createPortal is a better API, not everything should be hook-ify, the nature of using the component tree to render in createPortal is a nice feature.
|
|
||
| function Components( { subscriber, components } ) { | ||
| const [ , forceRender ] = useReducer( () => ( {} ) ); | ||
| subscriber.current = forceRender; |
There was a problem hiding this comment.
Writing refs during rendering is not recommended in concurrent mode. It seems to be only acceptable when it's for lazily evaluating for once.
There was a problem hiding this comment.
It seems to be only acceptable when it's for lazily evaluating for once.
What does that mean? forceRender remains constant here, so maybe it's fine?
| return () => { | ||
| components.delete( component ); | ||
| }; | ||
| } ); |
There was a problem hiding this comment.
This seems fragile. Every time it renders it deletes the component and adds a new one. And when it does that it changes the order of the components. Because of the lack of keys, it will just re-render everything.
| }, | ||
| delete( component ) { | ||
| components.current.delete( component ); | ||
| subscriber.current(); |
There was a problem hiding this comment.
Whenever a component that useRootPortal re-renders, it will delete the content and re-add it in useLayoutEffect, thus calling subscriber twice, which is just a forceRender function in the Components component. This results in a double-render for the portal component for every render of the consumer's component. While React is typically very fast, this does feel like an unnecessary overhead.
talldan
reviewed
Mar 7, 2023
| @@ -41,13 +46,55 @@ import { | |||
| DEFAULT_BLOCK_EDIT_CONTEXT, | |||
| } from '../block-edit/context'; | |||
|
|
|||
| const elementContext = createContext(); | |||
| const componentsContext = createContext(); | |||
There was a problem hiding this comment.
Should it be ComponentsContext?
This also confused me when I first noticed elementsContext, all the other contexts in the codebase have an uppercase first character.
There was a problem hiding this comment.
It's not a component? :)
youknowriad
reviewed
Mar 7, 2023
There was a problem hiding this comment.
It's a bit unclear from the PR description how this is better than the previous API. Maybe add some arguments
After reading the usage of this API a bit. It seems that actually a better one would be just a component (instead of a hook), Something like <BlockListRoot>something</BlockListRoot> (or BlockListFill to keep our semantics and naming conventions)
|
I guess my intent is not clear here. I created a new PR (#48884) which demonstrates why it's better to do this in a hook. |
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.
What?
I created the
__unstableElementContextcontext a while ago for Duotone, but since a lot of BlockListBlock filters have started using it.Why do we want to change it? It turns out that all BlockListBlock filters just want to change block props while sometimes adding styles or SVG that target the block by a selector. It would be good if we create a new, less powerful filter for just the block props and deprecating this way too powerful component level BlockListBlock filter.
The only thing that stands in the way is the need for portals, because they have to be rendered in the React tree. It would be great if we can add styles and SVGs to the root block list by simply calling a hook so it can be used in the future
useBlockPropsfilter.This PR does exactly that.
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast