Try making page list a controlled block #46416
Conversation
talldan
added
[Type] Code Quality
talldan
requested review from
draganescu,
scruffian,
getdave and
MaggieCabrera
|
🤔 One thing to consider is that if this PR goes ahead, Page List will suddenly appear in purple as a synced block - Experiment: Make block "synced" status applicable to all controlling blocks. In a way it's right, because Page List is synced to the site's pages, but it's still different from the way other synced blocks work. |
|
Size Change: -5 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
| @@ -133,6 +132,9 @@ export default function PageListEdit( { | |||
| renderAppender: false, | |||
| __unstableDisableDropZone: true, | |||
| templateLock: 'all', | |||
| onInput: NOOP, | |||
| onChange: NOOP, | |||
| value: blockList, | |||
There was a problem hiding this comment.
This is what we were missing @draganescu
There was a problem hiding this comment.
So we don't use templates we just set the inner blocks. Hmm how to have thought of this :/
@talldan do we need both template lock and the onChange and onInput NOOPs?
There was a problem hiding this comment.
@talldan do we need both template lock and the onChange and onInput NOOPs?
It didn't seem to work without defining onChange / onInput. Possibly a bug, or possibly intentional. 🤔
Template lock does make the fact that the block is not editable visibly noticeable, I wasn't sure if you wanted to keep it for that reason.
| @@ -60,7 +70,7 @@ const MenuInspectorControls = ( { | |||
| /> | |||
| { isOffCanvasNavigationEditorEnabled ? ( | |||
| <OffCanvasEditor | |||
| blocks={ innerBlocks } | |||
| blocks={ clientIdsTree } | |||
There was a problem hiding this comment.
This is the change the broke the edit button. Could it break other things that are relying on blocks to be an array of block objects rather than just client ids?
There was a problem hiding this comment.
This is a pretty significant change. Why is it necessary?
Also this will likely diverge us from the primary <ListView> which presumably accepts blocks?
There was a problem hiding this comment.
It's the other way around, the main List View uses a clientIdsTree. There's a bit more detail in the PR description.
I do think the blocks prop might be confusing, but the root cause is really that the innerBlocks data doesn't include controlled blocks.
There was a problem hiding this comment.
It's the other way around, the main List View uses a clientIdsTree. There's a bit more detail in the PR description
🤦 Apologies
|
The other thing this breaks is the ability to add a submenu item. Looking into that next... |
If you share the steps to repro that, then I can look into it. |
This was tricky to find. Use the 3 dots menu in the offcanvas experiment: Screen.Capture.on.2022-12-14.at.09-15-20.mp4That said @scruffian I'm not sure this should work. Rather it's a bug in the 3 dots menu which shouldn't show that control unless the block is suitable. A Page List Item block cannot have a Submenu block within it. If you think I'm right then we should spin up an Issue for this. |
There was a problem hiding this comment.
Thanks for looking to improve this.
✅ Conversion to links worked as expected using the Customize button in the inspector controls.
✅ Rendered pages worked as expected.
❌ Appending a new block using the offcanvas inserter (or indeed within the editor canvas) didn't trigger Page List to convert to Nav Links. The Nav Menu was created but Page List still remains as a Page List which is wrong (it shoudl convert to Nav Link items).
Screen.Capture.on.2022-12-14.at.09-45-23.mp4
| @@ -60,7 +70,7 @@ const MenuInspectorControls = ( { | |||
| /> | |||
| { isOffCanvasNavigationEditorEnabled ? ( | |||
| <OffCanvasEditor | |||
| blocks={ innerBlocks } | |||
| blocks={ clientIdsTree } | |||
There was a problem hiding this comment.
This is a pretty significant change. Why is it necessary?
Also this will likely diverge us from the primary <ListView> which presumably accepts blocks?
| useEffect( () => { | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| if ( blockList ) { | ||
| replaceInnerBlocks( clientId, blockList ); | ||
| } | ||
| }, [ clientId, blockList ] ); | ||
|
|
There was a problem hiding this comment.
We should also be sure that this change doesn't break any of the checking we did in #46223
It doesn't seem to happen in trunk, but I might be testing incorrectly. Could you share steps to repro (I don't know much about this feature)? |
Looks like I uploaded thw wrong video to my review. Basically active the experiment under Click the But...I just realised this problem is also probably in |
There are two different issues. The one I am talking about is adding a submenu item to a standard navigation link:
This does work in trunk because it makes some assumptions about innerblocks being defined on a |
|
@scruffian Shall we try and help @talldan narrow this one down as the offcanvas is a fairly niche project at this point? We should
Personally I would like to see the change to use the tree of client Ids separated into its own PR. |
yes! |
|
Let's merge #46541 and then rebase this one. Then I'll re-review and try and narrow down the source of these bugs. |
|
I created an issue for the submenu things so as to not block this PR on that: #46553 |
scruffian
force-pushed
the
try/page-list-as-a-controlled-block
branch
from
871a628 to
b5ffb89
Compare
|
This seems to be working well to me. |
Related #46330
What?
Tries making Page List a controlled block, as an alternative to using an effect to call
replaceInnerBlocks.Why?
This is mostly a code quality change, it reduces some complexity by removing an effect.
How?
Set the
valueof the inner blocks instead of usingreplaceInnerBlocks.onInputandonChangeare set to a noop because the block is not editable.Making this change also exposed an issue with the off-canvas editor, which is fixed here. It wasn't showing the controlled children of Page List. The implementation now uses
__unstableGetClientIdsTreeto match what the main List View implementation does.Testing Instructions
Everything should be the same as
trunk.Testing Instructions for Keyboard
The steps should be keyboard friendly. For keyboard testing, it may be easiest to use List View to select blocks, as selecting blocks in the canvas is still difficult.