-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update the block wrapper properly when the alignment changes #22099
Conversation
@@ -4,16 +4,17 @@ | |||
import classnames from 'classnames'; | |||
import { first, last, omit } from 'lodash'; | |||
import { animated } from 'react-spring/web.cjs'; | |||
import mergeRefs from 'react-merge-refs'; |
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 was technically not needed, but I believe this is better than the "fallback" we had previously. The previous fallback wouldn't have worked with functions as refs...
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.
Why do we need to support functions?
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.
we can just say we don't, but I think a React component user would expect any ref to work not just the ones created with createRef.
<Block.div ref={ node => dosomething } />
Also, this doesn't have any downside (including bundle size)
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.
Ok, sounds good
[ clientId ]: wrapper.current, | ||
} ) ); | ||
return () => setBlockNodes( ( nodes ) => omit( nodes, clientId ) ); | ||
}, [ isAligned ] ); |
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.
The fix here is that the block node is set now for all blocks (not just selected blocks) and is refreshed only when the "isAligned" changes because that's the only thing that can affect it here.
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.
Why do we need all block nodes for alignment?
I suspect that this will impact loading performance because this blocks layout. Perhaps we can use useEffect
instead, but I recall having some e2e test problems with that.
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.
Generally, I do think storing all reference would be useful, especially since it could replace the "ugly" querying of block nodes in other places. Not sure why we need it in this PR though.
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 checked the performance impact and I don't think there's one. That said I'd appreciate confirmation since there are fluctuations.
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.
Why do we need all block nodes for alignment?
Assigning or unassigning an alignment rerenders the block entirely and generates a new "node" for this block. (because there's an extra wrapper needed)
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.
Oh, does a component not remount when that happens?
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.
no, it doesn't, the element change but the component is still the same.
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.
Hm, I wonder if we should update the list on every rerender, if the node is not the same.
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.
Or maybe we should store the ref object rather than the element itself?
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.
the node only changes on mount/unmout and on alignment set/unset because the element tree changes at that moment. I tried using a "function as ref" because that's that function is called at the exact moment the ref changes but it doesn't work because that function is called at "render" time by React causing infinite loops and weird breakage.
What I have now is basically the same thing as "function as ref" but called as an effect (once the render is finished).
Size Change: +14 B (0%) Total Size: 822 kB
ℹ️ View Unchanged
|
There's an e2e test failure, I tried CPU debounce but I'm not able to reproduce locally :( |
c66b0bd
to
65a43c4
Compare
I see a big change in loading time. (make sure you run master:
pr:
|
Could we leave out setting all block nodes for now? I don't think it's needed for this PR? |
Ok, I created #22108, which removes |
65a43c4
to
07b3b24
Compare
So 😅 it is slower now... probably because |
@youknowriad Would you be ok with splitting out the fix (#22117)? |
@ellatrix It still seems like an optimization tailored for a particular use-case (toolbar) while the context is more generic "block nodes" but I agree with landing it first. |
About the performance impact, even if the context is updated more often, It seems that it shouldn't necessarily result in a lot more rerenders since, in theory, only the "consumer" (block toolbar) rerenders. There might be an optimization to be done elsewhere. |
So what we need:
This sounds a lot like |
if we were to use a store, it seems "block-editor" is the right one but I'm not sure why the context approach can't work similarly? maybe we just need to "memo" the block nodes provider's children or something like that. |
Yeah, it should be possible to do. I'm not sure if |
Ok, just quickly confirmed with a test that it is: https://codepen.io/iseulde/pen/YzyewKB |
07b3b24
to
6060366
Compare
Found a fix I think. We could wait to set the block node in the first idle period. That's ok, dependent on the node already have to wait for the information since it's not provided synchronically. We could potentially only delay it on the first render. |
useEffect( () => { | ||
if ( isSelected || isFirstMultiSelected || isLastMultiSelected ) { | ||
const node = wrapper.current; | ||
const id = window.requestIdleCallback( () => { |
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 function is not available on IE, we polyfill it somehow on @wordpress/priority-queue.
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.
Oh yeah, forgot about that
As expected, this triggers some |
the only potential downside is that you need to know that it's async on the consuming level (I mean don't assume that it's always available) |
You cannot currently make that assumption either, because it's only set in a |
There seem to be a few e2e test failure which are definitely caused by this code change (don't see them in master). |
I think we already fixed this differently. |
on master, when we update block alignment from not aligned to any alignment (left, right,....) or the opposite, the block toolbar disappears and don't appear only after deselecting/reselecting the block.
This PR fixes the behavior.
Testing instructions