List View: Add a new prop to allow blocks in the inserter to be prioritized #49959
Conversation
scruffian
added
the
[Feature] List View
|
Size Change: +308 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is mostly looking good to me code-wise, and I see it's largely copying over parts of #48752 from the off canvas editor to the list view.
Just left a couple of comments: whether we need to do the unlocking since we're already in the block editor package, and whether we could neaten it up slightly by using the context in the list view rather than prop drilling.
I'll just add @ntsekouras as a reviewer since I think the unlocking was added to the off canvas editor in #48752. I very well mightn't be understanding how the unlock behaviour is meant to work, so happy to go with this if it's the right approach!
| @@ -99,6 +99,7 @@ function ListViewBranch( props ) { | |||
| shouldShowInnerBlocks = true, | |||
| isSyncedBranch = false, | |||
| showAppender: showAppenderProp = true, | |||
| prioritizedInserterBlocks, | |||
There was a problem hiding this comment.
To avoid prop drilling, could we add prioritizedInserterBlocks to the list view context around here?
That way the appender can know about it (and grab it from the list view context), rather than the branches needing to be aware of it.
| const descriptionId = `list-view-appender__${ instanceId }`; | ||
| const { PrivateInserter } = unlock( blockEditorPrivateApis ); |
There was a problem hiding this comment.
Why is this being unlocked via the block editor private APIs? We're currently in the block editor, so I thought we could import directly from that component, as it's a relative import rather than reaching out to a separate package.
There was a problem hiding this comment.
You're right Andrew, thanks! It's not needed and I opened a PR to fix the current usage.
There was a problem hiding this comment.
Let's rebase to pull in this changed.
|
I'm not really sure this makes sense as an API of ListView. Why does only the quick inserter in List View have prioritised blocks, and not the quick inserter in the canvas? Instead I would expect this as part of the There are probably some other use cases where this could be a useful feature. An example is when inserting into a header template part—really the Site Title, Site Logo, Navigation, Social Links and maybe a few others are the most likely blocks to be inserted, but at the moment you tend get paragraph and other commonly used blocks as the suggested blocks. |
|
I'd echo @talldan 's concern. The introduction of this private API was really close to 6.2 release, in an attempt to fix some blockers for navigation in site editor and off canvas appender. After that it was pulled off from the site editor, so it might make sense to explore a better API before expanding on this one. |
|
I agree with @talldan's proposal and @ntsekouras's concerns. However, it's becoming ever more important that we get rid of the It represents tech debt that needs to be cleared out ASAP as it places an unwanted burden on contributors' time which could be better spent elsewhere. As a suggestion could we first focus on marrying the two implementations, leaning on private APIs to normalise, so that we can remove Then once complete, we can look to remove the private API for I'm not saying that the API on InnerBlocks is necessary terribly difficult, but at this point it's unknown and could potential spiral into rounds of PR reviews. Given that we can safety lean on private APIs to hide functionality we're not ready to expose, it seems expedient to focus on removing |
|
I think the path from @getdave is sensible at this stage strictly because it's code that can be refactored to use new/better APIs when we have them (new inner blocks API as @talldan suggests or new inserter API as @ntsekouras suggests), which have known complexity, as this serves the immediate goal of not increasing useless technical debt, which has unknown complexity. |
getdave
force-pushed
the
add/inserter-block-sorting
branch
from
55e8682 to
a1c0c6f
Compare
|
Flaky tests detected in e39e499. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4926964293
|
|
@getdave @draganescu Are there any PRs exploring the alternative options that were proposed? The hardest part with them is probably getting buy-in on a new API, but I think the best place to start is a PR. I guess it's ok adding this as a private API, though it feels like moving tech debt from one place to another. I think this was always going to be the problem with using a separate OffCanvas component—a long tail of tech debt to handle. It might have ended up quicker to deliver the feature, but might have ended up with more developer work. |
Just to add, the rounds of PR reviews here are saying "this isn't recommended". Yet it seems like you're proposing to go ahead that here and still ship this as a private API. The only difference is that this is known and the others are unknown, so the right thing IMO is to expend some time to make the other options also known (and those could also still be private APIs). Then the best option can be chosen based on the trade-offs. Here you're effectively forcing a resolution by not being willing to entertain any alternative. |
The only reason we're in such a "rush" is because the work done in the navigation block inside the off canvas editing experiment is now suddenly reused in the site editor, a much higher scope, much more exposed, for a new sort of feature. This was completely unknown at the time. Should we have been more explicit in don't reuse this experimental component? Is the connect between navigation tree editing in the inspector, and editing of the site wide navigation menus, in a place where we have preview and a completely different UX, really warranting reusing the stuff from the navigation block, considering that not even the navigation block work is completely polished yet (see link UI and the mahjon popovers to add a submenu/link)? I don't know. I think there are two valid points pulling in different directions:
Personally I think we should not use the off canvas in the site editor navigation sidebar - and just use the list view and whatever the list view needs and doesn't have is reflected in the site editor navigation sidebar not working as needed - yet! We should keep thinking in the context of Finally, about this particular PR I think we also have a miscommunication of sorts, so let's remember we all mean well ☮️
|
|
Since #50287 is now merged we no longer have to worry about OffCanvasEditor being used in the global navigation sidebar (aka browse mode). I think this gives us a bit more time to explore the other solutions for how we might prioritize blocks in the inserter without feeling so much pressure to get something shipped ASAP. We can at least explore that idea and see how complex it looks... I'll dig into it now... |
|
Closing in favour of #50510 |
What?
One feature we added to the OffCanvasEditor component was the ability to sort the blocks in the Inserter, so that we can give access to the most commonly used ones. This PR adds that same functionality to the List View using a private prop called
prioritizedInserterBlocks.Why?
How?
Adds a private prop called
prioritizedInserterBlocks. We could potentially use the existingshowAppenderprop, and pass an array to it, but I think this way is simpler and easier to understand.Testing Instructions
This isn't possible to test using this PR, but the same commit is in this PR: #49417, so you can test it there.
To test:
Testing Instructions for Keyboard
As above
Screenshots or screencast