Add new API to allow inserter items to be prioritised #50510
Conversation
|
Size Change: -49 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
|
@scruffian As requested (away from Github) I reworked this to use a new API on inner blocks called Why? Because I thought if we reuse Note that the I guess we might need to do the same for the variations though, in case someone includes a variation of a block that isn't in the allowed list. |
|
Flaky tests detected in 259a23d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4981077120
|
packages/block-editor/src/components/inserter/search-results.js
Outdated
Show resolved
Hide resolved
getdave
changed the title
getdave
added
[Feature] Nested / Inner Blocks
packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Outdated
Show resolved
Hide resolved
| orderInitialBlockItems={ undefined } | ||
| /> | ||
| ); | ||
| return <ComposedPrivateInserter ref={ ref } { ...props } />; |
There was a problem hiding this comment.
We can probably follow up to remove the need for the private wrapper once (and if!) this PR is merged.
There was a problem hiding this comment.
I would keep the private wrapper. This shape allows for easy future private APIs without all the boilerplate being re-added.
There was a problem hiding this comment.
I was looking for why we haven't removed the private inserter. IMO, this doesn't seem like a good reason to keep things around just in case we might need them in the future.
packages/block-editor/src/components/inserter/search-results.js
Outdated
Show resolved
Hide resolved
|
As this represents a change to a key API, I'm pinging @youknowriad for review as well. |
| <ComposedPrivateInserter | ||
| ref={ ref } | ||
| { ...props } | ||
| orderInitialBlockItems={ undefined } |
There was a problem hiding this comment.
I suppose the other option is to keep this API, but instead support passing a custom appender to List View (instead of the showAppender prop).
It still doesn't solve the issue where the block itself ends up with different ordering, unless both the block and the list view instance use the same custom appender, so I think what's in this PR is better. It's define once and it works everywhere which means there's less room for errors.
There was a problem hiding this comment.
I think
instead support passing a custom appender to List View (instead of the showAppender prop).
is better than the prop, but why would we need to keep orderInitialBlockItems?
There was a problem hiding this comment.
Only if you wanted specific instance X of an inserter to have a specific order.
The new implementation in this PR is a blanket application. Any block that uses the new prop on InnerBlocks no matter where it is with have items prioritised.
The previous implementation was more targetted.
There was a problem hiding this comment.
Wouldn't orderInitialBlockItems be the default order for when block's don't prioritize or for the generic canvas inserter in an empty paragraph?
Edit: no, that's for orderInitialBlockItems in the search results ro handle. This orderInitialBlockItems prop can be removed because it's effectively replaced by a public API doing the same thing.
There was a problem hiding this comment.
Thank you for exploring this. It may have taken longer, but it feels like the better option to me.
I didn't have a chance to do a full code review, but the code looks like what I'd expect.
I'm not sure if it could be made private, but it also feels like a really useful feature to me, so it could shipped stable. Tests and docs might be needed once there's an agreement on this.
|
If I'm not wrong @ntsekouras did some work in the past for internal ordering in the inserter. So he might have some feedback here. |
packages/block-editor/src/components/inserter/search-results.js
Outdated
Show resolved
Hide resolved
|
This looks excellent IMO. it solves the block inserter being tailored to what the block accepts as inner blocks and to what the most common UX is. |
| return [ ...new Set( inserterItemTitles ) ].sort(); | ||
| return [ ...new Set( inserterItemTitles ) ]; |
There was a problem hiding this comment.
I changed this in 8cb75c8.
Because it does sorting on the results the tests cannot assert on the priority of the blocks.
This helper is "doing it wrong". Instead it should return raw results and the consumer should do the sorting.
Let's see which tests break before deciding how to proceed from here.
There was a problem hiding this comment.
Do we think this should be it's own sub PR?
All E2E tests passed in the Gutenberg Mobile PR (CI job reference) 🎊 . Note that the PR gets automatically updated when a new commit is pushed in this PR, so the tests will be re-triggered with next commits. |
| templateLock, | ||
| captureToolbars, | ||
| orientation, | ||
| layout |
There was a problem hiding this comment.
Why is this PR adding all these other settings and not only the core feature prioritizedInserterBlocks?
There was a problem hiding this comment.
Because not doing that broke the Mobile implementation (cc @fluiddot).
| style: { outline: '1px solid gray', padding: 5 }, | ||
| }; | ||
|
|
||
| // Make it easier to select the block. |
There was a problem hiding this comment.
What does this comment mean?
There was a problem hiding this comment.
It was indicating that without a placeholder within the innerblocks it can be difficult to select the block using e2e tests, especially using Puppeteer. Open to suggestions for improving the comment 🙇
There was a problem hiding this comment.
without a placeholder within the innerblocks it can be difficult to select the block using e2e tests, especially using Puppeteer, so we use an image block which has a placeholder.
packages/e2e-tests/plugins/inner-blocks-prioritized-inserter-blocks/index.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Outdated
Show resolved
Hide resolved
…ettings-update.js
|
I updated the docs. I say lets merge this and make sure to follow up with the slash inserter fix ASAP. |
|
|
||
| await page.waitForSelector( QUICK_INSERTER_RESULTS_SELECTOR ); | ||
|
|
||
| await expect( await getAllBlockInserterItemTitles() ).toHaveLength( |
There was a problem hiding this comment.
I noticed this test in the follow up PR and it doesn't seem to add any value. We should either test something about the existence/non-existence of some blocks or remove it.
There was a problem hiding this comment.
Yes I agree. Seems pointless. It was probably me copying boilerplate code about.
We should probably change it to test what happens if you pass an empty value or empty array as the prioritized blocks argument.
bph
added
the
Needs Dev Note
What?
Adds a new API to the block list settings to allow parent blocks to define a priority for blocks in the inserter for a given inner blocks.
Why?
In #48752 we added a private API to allow blocks to be prioritised. This was so we could force
Page LinkandCustom Linkto show up as the first two results in the inserter options for the Navigation block's list view editing mode.However, this was always envisaged as a short term way to allow the wider feature to land in WP 6.2. This PR circles back (and is an alternative to #49959) with an alternative approach which looks at this problem more holistically by assuming the requirement to prioritise blocks will likely be useful to other block's in future.
How?
Adds a new
inserterPriorityproperty to inner blocks which allows the consumer to pass an array of blocks and/or variations which should be prioritised in the Inserter.It also adds a concrete example, by way of alternating the Nav block to utilise this new property to cause the
Page LinkandCustom Linkblocks to show up first in the Inserter.Testing Instructions
#### Automated Testing
Run
Manual Testing
INSERTER_PRIORITYconstant inconstants.js(see changeset) to be:npm run devcore/spacerblock as first item followed by the other two block variations.Testing Instructions for Keyboard
Screenshots or screencast
Co-authored-by: Dave Smith 444434+getdave@users.noreply.github.com