Components: Try to make the order of fills stable in regular slots #29287
Conversation
gziolo
requested review from
adamsilverstein,
ajitbohra and
chrisvanpatten
as code owners
| @@ -54,6 +55,12 @@ class PluginArea extends Component { | |||
| super( ...arguments ); | |||
|
|
|||
| this.setPlugins = this.setPlugins.bind( this ); | |||
| this.memoizedContext = memoize( ( name, icon ) => { | |||
There was a problem hiding this comment.
While it decreases the number of rerenders it doesn't quite solve the issue when using the concept of occurrences.
There was a problem hiding this comment.
Is this related to ordering, or did we notice a chance to reduce rerenders?
There was a problem hiding this comment.
It doesn't have any impact on the ordering but prevents excessive rerenders. I suspected initially it might cause some random unmounting, but my tests didn't confirm that.
|
Size Change: +194 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
PR in the actual form resolves the issue reported by @jasmussen in #28546: Screen.Recording.2021-02-24.at.08.12.35.movIt also works with dynamically registered plugins that declare a corresponding pinned item: Screen.Recording.2021-02-24.at.08.21.10.mov |
|
It looks like the order of fills in the inspector controls is predictable as well: Screen.Recording.2021-02-24.at.08.37.03.movAll e2e tests pass. @youknowriad – what do you think about the changes proposed? The flaw of the previous implementation is that as soon as a fill is unregistered, all other fills get their Behavior in the main branch: Screen.Recording.2021-02-24.at.09.41.42.mov
|
gziolo
added
the
[Feature] Extensibility
|
Thanks so much for working on this! This is what I see:
This appears to work excellently well for me! |
|
I did another test using the approach from #22027 where slots use diff --git a/packages/interface/src/components/pinned-items/index.js b/packages/interface/src/components/pinned-items/index.js
index 59fc700030..d15602414b 100644
--- a/packages/interface/src/components/pinned-items/index.js
+++ b/packages/interface/src/components/pinned-items/index.js
@@ -7,28 +7,32 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
-import { Slot, Fill } from '@wordpress/components';
+import {
+ Slot,
+ Fill,
+ __experimentalUseSlot as useSlot,
+} from '@wordpress/components';
function PinnedItems( { scope, ...props } ) {
return <Fill name={ `PinnedItems/${ scope }` } { ...props } />;
}
function PinnedItemsSlot( { scope, className, ...props } ) {
+ const name = `PinnedItems/${ scope }`;
+ const slot = useSlot( name );
+ const hasFills = Boolean( slot.fills?.length );
+
+ if ( ! hasFills ) {
+ return null;
+ }
+
return (
- <Slot name={ `PinnedItems/${ scope }` } { ...props }>
- { ( fills ) =>
- ! isEmpty( fills ) && (
- <div
- className={ classnames(
- className,
- 'interface-pinned-items'
- ) }
- >
- { fills }
- </div>
- )
- }
- </Slot>
+ <Slot
+ { ...props }
+ name={ name }
+ bubblesVirtually
+ className={ classnames( className, 'interface-pinned-items' ) }
+ />
);
}It's better than in the main branch in terms of the order of pinned items that is predictable when registering a new plugin, However, the order changes when unpinning and pinning the sidebar, and goes back to the previous order when refreshing the page: Screen.Recording.2021-02-24.at.10.19.29.mov@diegohaz, any hints on how to ensure that the order of fills doesn't change when using |
|
I guess for |
|
Maintaining the order of elements with React Portal is tricky. There's a related discussion here: #19242 (comment) I was experimenting with the Rendering an empty element as @youknowriad suggested looks like a good alternative too. But I think there will be some styling issues ( |
gziolo
force-pushed
the
try/fix-order-fills
branch
2 times, most recently
from
272d3e5 to
255928e
Compare
I'm afraid there is more. There is logic in many places that render wrappers for the slot if there are no fills rendered. At the moment, when fills render nothing (something falsy like const slot = useSlot( name );
const hasFills = Boolean( slot.fills?.length );@youknowriad, how about we land this PR with the latest updates that contain unit tests for the slot that doesn't bubble virtually. They clarify how the slot behaves with fills that are always mounted and what happens when a fill gets unmounted. In the long run, we should try to migrate all slots to bubble virtually as the default for the web, and use the other implementation for the mobile (RN). |
| @@ -109,19 +108,13 @@ class SlotFillProvider extends Component { | |||
| if ( this.slots[ name ] !== slotInstance ) { | |||
| return []; | |||
| } | |||
| return sortBy( this.fills[ name ], 'occurrence' ); | |||
There was a problem hiding this comment.
What problem were we solving earlier that we needed to count instances?
There was a problem hiding this comment.
It's very difficult to tell even when you go through past commits and related discussions. There was definitely something that is covered with the unit test that I updated. It was discussed in some of the PRs I studied but there wasn't a clear answer why this particular test needs to be kept:
gutenberg/packages/components/src/slot-fill/test/slot.js
Lines 170 to 209 in 9111ad5
I extended this test to see how it behaves in the trunk branch, and I realized that the test is too specific and doesn't cover more complex cases. The updated version of the test produces the following order that is far from perfect:
<div>
third
first (rerendered)
second
fourth (new)
</div>
The code of the test:
gutenberg/packages/components/src/slot-fill/test/slot.js
Lines 231 to 273 in 255928e
There was a problem hiding this comment.
Curious, if we can't remember, it does make sense to update the expectation here
There was a problem hiding this comment.
<div> third first (rerendered) second fourth (new) </div>
@gziolo Does this happen on the main branch or only on this PR? I think rendering Fills conditionally (instead of just omitting its contents) like in that test is a pretty common thing.
There was a problem hiding this comment.
@diegohaz, I copied the result from the main branch. I wrongly assumed that it's easy to find the result in the diff section for the same test, that is in this branch:
<div>
second
third
first (rerendered)
fourth (new)
</div>
It's slightly better as more predictable, but it's still far from perfect. This is also why fills for PinnedItems are optimized to be always rendered, and they conditionally show its content. This way, the order doesn't change in my testing, and as I can tell, it can be confirmed by others.
Aside: It aligns now with the behavior of the version that bubbles virtually, the advantage is that you can keep the fill always rendered. Ideally, we should improve the version that bubbles virtually and use it everywhere. In the meantime, I'm planning to improve the implementation to ensure that mobile (React Natvie) uses the version without portals by design.
| @@ -54,6 +55,12 @@ class PluginArea extends Component { | |||
| super( ...arguments ); | |||
|
|
|||
| this.setPlugins = this.setPlugins.bind( this ); | |||
| this.memoizedContext = memoize( ( name, icon ) => { | |||
There was a problem hiding this comment.
Is this related to ordering, or did we notice a chance to reduce rerenders?
gziolo
force-pushed
the
try/fix-order-fills
branch
from
255928e to
24e7cf9
Compare
Yes, it's a very complex piece of software and crucial to how many things are architected 😄 I did most of my tests using You can also install some plugins like @jasmussen: Jetpack and Yoast. I would say you need at least 3-4 different plugins to notice this behavior. |
There was a problem hiding this comment.
Thanks for looking into this @gziolo! Giving tentative approval here, since I didn't spot any issues while smoke testing in a few places. Getting another +1 review for a confidence check would be helpful.
This is a little bit on the riskier side to land, since we don't remember the original reason for counting fill instances. I'm in favor of removing the instance counting if it doesn't actually fix a case. If we decide to land, maybe pick a time where folks can keep an eye on any behavior differences.
|
I'm merging this PR hoping that it will help to better align the behavior between two versions of SlotFill implementation. Let's observe if it doesn't cause any regressions in the block toolbar or block inspector. Most importantly this PR fixes the issue with the unstable order of pinned items that is very confusing for users as reported by @jasmussen. |
|
Thank you 🚀 |
|
I left a message on WordPress Slack about this change (link requires registration at https://make.wordpress.org/chat/): |
Description
This PR tries to resolve #28546.
How has this been tested?
Register a few sidebar plugins that are pinnable, for example, plugins that offer integration: Yoast, Jetpack. Make sure that the order of pinned items remains the same when you pin and unpin them several times.
Screenshots
Screen.Recording.2021-02-24.at.08.21.10.mov
Types of changes
Enhancement.
Checklist: