Insert Links by default in Navigation block #34899
Conversation
tellthemachines
added
[Block] Navigation
|
Size Change: +216 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Your stamina to take one difficult ticket after another is impressive. Thank you. It's already looking good:
As is, this feels very validating towards the concept as a whole. Working towards the second half, transforms, it's already becoming clear that showing the 3 most recent items published in the page selector, requiring search for anything else, isn't that useful. This is almost certainly not something that should be solved in this PR, and there are some mockups in this ticket, but it feels like a worthwhile next step. Let me know if/how I can help with this one. |
|
Thanks for looking into this @tellthemachines ! I gave this a try, and one thing I quite like about how the buttons container works currently is that it quickly lets us add our placeholders before we need to fill in the details. In the context of the navigation editor, this branch lets us add items pretty quickly, but I got pretty distracted by the link search that auto opens. If we're trying to quickly build a submenu, this makes it a little more apparent that it can get in the way if I'm building a menu in that way. If we prevent the link search from auto opening, a natural thing to do might also be to add an option to the link search to open the quick inserter if we're not interested in a link:
CleanShot.2021-09-20.at.13.17.23.mp4Note that if this PR ends up being viable, we can also use the method here to improve how social icons inserts items in a follow up✨ |
If we want page links, we'll need to add the following attributes to make sure we get the Select Page prompt:
|
Updated to Page items by default, but I now realise that this complicates the flow for inserting other types of links: searching for e.g a category name won't show any results. In optimising for Pages we risk making the general experience worse instead of better 😕
The link control auto-opening is what happens currently when adding a Link block, so changing that isn't in scope for this PR. In any case, I'm not convinced we should change it. Nav links are different to buttons in that they are most often based on existing pages or posts, so it doesn't make sense to add a text placeholder for them and go back later to add the actual link. That would just be twice the amount of work 😅 I'm going to mark this one ready for review, but I'll hold off on updating the e2e tests until I have some more feedback on Page items by default, because going with that will force us to change quite a few things. |
tellthemachines
requested review from
ajitbohra and
ellatrix
as code owners
tellthemachines
requested review from
noisysocks,
talldan,
draganescu and
getdave
| () => ( onlyLinkInnerBlocks ? DIRECT_INSERT : [] ), | ||
| [ onlyLinkInnerBlocks ] | ||
| ); | ||
|
|
There was a problem hiding this comment.
This is memoized to stop the directInsertValue array from getting recreated on every render, and causing an infinite loop because it changes innerBlocks, which in turn the Navigation function depends on to calculate the directInsertValue 😖
I can't think of a better way to do it though. I think this logic should reside in the block, because it might vary a lot - Submenus are set to always use it (at least until we sort out mega menus), and conceivably there could be blocks using it depending on their nesting status or some other condition.
|
I have mixed feelings. Inserting pages is fast and easy, and is probably what most users will want to do, so that's great. But it also feels like the other options are very hidden and can only be added in a very indirect way. There are probably ways to mitigate this. Perhaps by improving the Link UI (I think @jasmussen did some explorations) or I think @mtias mentioned making the slash inserter usable (though you can't just type into a Page block, a link has to be selected first) alongside other ideas in the issue - #34041. |
| @@ -504,6 +509,7 @@ export default function NavigationSubmenuEdit( { | |||
| }, | |||
| { | |||
| allowedBlocks: ALLOWED_BLOCKS, | |||
| directInsert: DIRECT_INSERT, | |||
There was a problem hiding this comment.
There are existing mechanisms in variations and blocks to define whether they can be used in the inserter. Introducing some level of granularity there might be an alternative to this prop.
e.g.
"supports": {
"inserter": "__experimentalQuick" | "__experimentalGlobal" | true | false
}
There was a problem hiding this comment.
I remember @gwwar also looked into a default block for the navigation block, so going back to that might also be an option.
There was a problem hiding this comment.
The related PR was here, where we explored adding a default block per container (vs one for the entire editor). #29490.
If I recall we ran into some of the same issues, where we made a guess as to what folks wanted to insert, but transforming blocks after that was more difficult.
There was a problem hiding this comment.
Let's highlight the default block better in the quick inserter.
🤔 From #34041 it might be worth trying an __experimentalDefaultBlock block list setting again.
I'll try tidying that experimental branch to see if its viable.
There was a problem hiding this comment.
There are existing mechanisms in variations and blocks to define whether they can be used in the inserter. Introducing some level of granularity there might be an alternative to this prop.
I don't think a setting in the block.json will work in this case, because we need to change it based on the block's innerBlocks. If any of the innerBlocks are not Nav Links, the quick inserter should appear instead of directly inserting a Link.
For the same reason, I'm not sure we'd want to leverage @gwwar 's __experimentalDefaultBlock, because the Insert before and Insert after buttons would be tied to its value. If we use __experimentalDefaultBlock, as it stands in #29490, to tell the block inserter when we want a default block to be inserted directly, we'll end up disabling Insert before and Insert after whenever the Navigation contains anything other than Links, which is quite unexpected.
Independently of whether we do enable Insert before and Insert after on all block containers in #29490 (which I think we should! it makes total sense) we still want to make it easier to insert Nav Links inside the Nav block, and a workflow that forces us to look for a deeply hidden option in the block toolbar doesn't satisfy that requirement.
In order to keep useInnerBlocksProps simple it would be nice if we could leverage the same prop for both use cases, but I'm not sure it will be possible. Am keen to know if anyone has other ideas around this though!
There was a problem hiding this comment.
That part of it is pretty complicated. There's still this part of the issue that indicates a way to designate a default block within the nav block will be needed:
Let's highlight the default block better in the quick inserter. It should always come in first. A user should not have to think when they open a quick inserter if they are not looking for a specific block.
I don't know that the current directInsert prop will work well with that.
Maybe an option is to look at using renderAppender to build out a lot of the custom insertion behavior first before formalising it into props and configuration.
There was a problem hiding this comment.
I'm not sure if __experimentalDefaultBlock is viable, but the idea of a default block other than a global one for the editor may still be useful. (The buttons block inserter cheats a bit since only one child item is allowed and doesn't apply well to other situations).
I mostly just rebased the branch yesterday and resolved conflicts. I'll ping y'all if I think there might be viable flow to look at.
There was a problem hiding this comment.
Maybe an option is to look at using renderAppender to build out a lot of the custom insertion behavior first before formalising it into props and configuration.
Not sure if I'm missing something, but renderAppender seems to be used only for determining what the appender looks like. It doesn't feel right to mix that with its behaviour (whether it should display quick inserter or insert a block directly), because we'll want to be able to set both independently.
There's still this part of the issue that indicates a way to designate a default block within the nav block will be needed
This is a good point. I'm thinking we could have two props: a defaultBlock array that specifies the default block name and any relevant attributes, and a directInsert boolean that specifies whether the default block, if it exists, should be inserted directly or not. That way defaultBlock can be used to specify the default for the quick inserter, as well as for insert before and after. If I'm not mistaken defaultBlock will need to be passed to useInnerBlockProps for all of these scenarios.
There was a problem hiding this comment.
@gwwar I'm going to steal the__experimentalDefaultBlock prop and give it a go in this PR 😂
We're looking at this in #33849 (comment). |
There was a problem hiding this comment.
Thank you for working on this.
I recognise the need for a lighter and faster experience for adding links.
My instinct is that this may not be the right solution. It makes adding Pages fast but it makes adding any other kind of link variation far more obscure.
One option we may not have considered is enabling some kind of "bulk addition" UI as a complementary alternative to the inline inserter. We're actually looking into this at the moment in the Navigation Editor and I wonder whether perhaps this ability to insert any type of item in bulk without triggering the Link UI (which I'm happy to work on or make configurable btw!) would also benefit the block?
I also think it would be worth exploring the scenario where:
- clicking plus auto-inserts a
core/navigationlink block without triggering the Link UI. - you can repeatedly add navigation link items without multiple clicks to close the link UI.
- once you're done with the "adding items" bit you can click on any item to "configure it" (we could show a visual indicator this is required).
I think this is what @gwwar suggested above. That would be a nicer flow in my opinion. It would have two (conceptual) "modes":
- insertion mode (I'm adding items).
- configuration mode (I'm setting where the items link to).
Thanks for working on this. I'm going to go away now and look at home to make the auto-appearing link UI popup configurable. I did an experiment #35001
|
I like this idea suggested by Dave
|
As I commented in #35001 (comment), I feel switching the insertion flow and asking users to first add all links, then set them, is a hard ask and not the smoothest insertion flow. If we want to go for a bulk-add hybrid approach instead, how about we explore adding a different appender to "insert another" of the same kind? This would allow, for example, inserting successive page links right after adding a first one, so that creating simple link menus is very straightforward but without obscuring the ability to add other types of blocks. |
|
The way I understand the discussion is this:
Can we eat a cake and have it too? Imagine a link UI that shows anything that matches: pages, posts, tags, categories, you name it:
Once you choose a tag, it becomes a tag link. If you edit it, it could still show all results for consistent experience – this way you could easily change it into a page link. Would that work, or am I missing something here? |
|
I second @adamziel's suggestion, but with a slight twist. I think it could work by:
|
There was a problem hiding this comment.
This is looking good so far @tellthemachines! 💖 The interactions feel good to me in both the post and navigation editor.
What we need next is updated tests and maybe a few more eyes on the new API. I left a few suggestions, but I'm curious what others think too.
| directInsertBlock[ 0 ], | ||
| directInsertBlock[ 1 ] | ||
| ) | ||
| : createBlock( allowedBlockType.name ); |
There was a problem hiding this comment.
I think the default block can work as an array or object. It may help to give a few more hints on the data structure with something like:
const [ name, attributes, innerBlocks ] = defaultBlock;
createBlock( name, attributes, innerBlocks );There was a problem hiding this comment.
Can you think of a scenario where we'd need defaultBlock to be an object? In the existing use cases (both this and your other PR) we're using it to store values to pass to createBlock so it makes more sense as an array.
In this PR, it's defined as an array in the jsdocs for the parameters passed to useNestedSettingsUpdate and the return value of __experimentalGetDirectInsertBlock. We'd need to change those if we wanted to support an object too.
There was a problem hiding this comment.
For me, a good reason to make it an object would be passing around a block instead of an array of arguments for createBlock. For example, this API could look like const DEFAULT_BLOCK = createBlock( 'core/navigation-link' ). One thing I don't like there is creating a uuid and a few other things we don't really need.
Maybe that could be a function then? For example const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-link' )
There was a problem hiding this comment.
🤔 Good point, I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks. A function would make sense in that case, though we might want to pick a friendlier name. (I find that some folks get confused by the creator name). Maybe INSERT_DEFAULT_BLOCK or something similar?
Edit: Maybe a simple name like getDefaultBlock? One other piece of this is we'll likely need to modify the suggested items in the quick inserter as part of 34041. (The quick inserter is currently ordered by block frecency).
There was a problem hiding this comment.
Hmm, I'm not so sure about this. When allowedBlocks is in use, the block is created directly in the inserter. I'd prefer to emulate that logic: fetching a block type, and creating the block from that type in the inserter. Default block and allowed block shouldn't be too different, because they essentially serve the same purpose. The main difference is that only one default block may exist even if multiple blocks are allowed.
I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks
I'm not sure I follow. We shouldn't have any reason to pass innerBlocks to the inserter, because it should always insert a brand new block.
There was a problem hiding this comment.
I suppose folks will need to use createBlock for complex innerBlocks and we'll want to avoid keeping around a static uuid for blocks
I think @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:
const DEFAULT_BLOCK_CREATOR = () => createBlock( 'core/navigation-menu', {}, [
createBlock( 'core/navigation-link', { title: "Home" }, [ ] ),
createBlock( 'core/navigation-link', { title: "About" }, [ ] ),
] )But as you say, I don't think it's a use-case that exists today.
Default block and allowed block shouldn't be too different, because they essentially serve the same purpose.
Hm I just thought of a completely different perspective here. Could we just make it a string to point out which supported block do we want to insert by default? It must be one of the supported blocks and we don't specify any attributes at the moment so maybe we don't need anything more? Or are we running in circles now?
There was a problem hiding this comment.
I think @gwwar referred to pre-populating that block with inner blocks. For example, that could be a pre-populated menu like this:
That's right. A better example might be a container block that makes little sense without children, for example the buttons block.
Since we don't have an explicit case for passing though attributes or innerBlocks yet, I think it's fine deferring needing to pass through extra arguments until we need them. Whichever combination you prefer @tellthemachines
There was a problem hiding this comment.
const blockToInsert = directInsertBlock?.length
? createBlock( ...directInsertBlock )
: createBlock( allowedBlockType.name )One thing to note is that if folks pass through innerBlocks as the third item in an array, it's possible to have innerBlocks with static uuids. It's an edge case, but two options I can think of:
- clone the block
insertBlock( cloneBlock( blockToInsert ), getInsertionIndex(), rootClientId );- Avoid passing through innerBlocks if we don't want to handle this case yet
const [ name, attributes ] = directInsertBlock;
createBlock( name, attributes )| ( block ) => | ||
| block.name === 'core/navigation-link' || | ||
| block.name === 'core/navigation-submenu' | ||
| ); |
There was a problem hiding this comment.
Have we considered allowing __experimentalDirectInsert to be a match function which returns true or false? It doesn't look like we need any other information specific to the render function. One benefit is that we can then make it a constant and can define it next to the default block.
For example it might look like:
const DIRECT_INSERT = ( block ) => {
return block.innerBlocks.every( ( { name } ) => name === 'core/navigation-link' || name === 'core/navigation-submenu' );
};And in the selector we can update it to call the match function.
There was a problem hiding this comment.
Oooh nice! I'll try it out.
There was a problem hiding this comment.
Hmm, this doesn't work well when we either add or remove other block types, because the selector won't re-run with every block update, so it won't know when the content has changed. Whereas if we keep the logic inside the edit function it updates with every change in the inner blocks.
There was a problem hiding this comment.
Have you tried without createSelector? We should get proper results provided we recalculate whenever the block.innerBlocks reference updates.
Edit: I see some earlier comments about an infinite loop. I'll 🔍 a bit. This is more of a nice to have if we can define this outside.
There was a problem hiding this comment.
The following works for me. (I didn't notice any infinite loops, but let me know if there's a specific case to test.)
Non-memo'd:
export const __experimentalGetDirectInsertBlock = (
state,
rootClientId = null
) => {
if ( ! rootClientId ) {
return;
}
const settings = state.blockListSettings[ rootClientId ];
const defaultBlock = settings?.__experimentalDefaultBlock;
if ( ! defaultBlock ) {
return;
}
const directInsert = settings?.__experimentalDirectInsert(
getBlock( state, rootClientId )
);
if ( ! directInsert ) {
return;
}
return defaultBlock;
};Or memo'd
export const __experimentalGetDirectInsertBlock = createSelector(
( state, rootClientId = null ) => {
if ( ! rootClientId ) {
return;
}
const settings = state.blockListSettings[ rootClientId ];
const defaultBlock = settings?.__experimentalDefaultBlock;
if ( ! defaultBlock ) {
return;
}
const directInsert = settings?.__experimentalDirectInsert(
getBlock( state, rootClientId )
);
if ( ! directInsert ) {
return;
}
return defaultBlock;
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.blocks.tree[ rootClientId ],
]
);There was a problem hiding this comment.
Oh I see, I was forgetting to add state.blocks.tree[ rootClientId ] to the dependencies 🤦
I think we should also accept a boolean value though, to avoid having to do something like () => true when we want to always insert the default block.
There was a problem hiding this comment.
I think we should also accept a boolean value though
That sounds reasonable to me 👍
Direct insertion depends on the block for now, though conceivably we could change it to be dependent on context if necessary. Right now it isn't because we want the same behaviour in all the editors.
Thanks for the review @gwwar ! I've updated the failing tests and added a new test to check the quick inserter still works when non-link blocks are present. Would welcome more feedback on the API! |
There was a problem hiding this comment.
Thanks @tellthemachines I think we're in pretty good shape here. This tests well for me manually and the test updates make sense. ✨
We may want a +1 review from folks working on the Navigation Editor since this effectively makes it pretty difficult to add a link variation. The only flow I found for that was using the global inserter.
Here's what I see. Note that the navigation crash at the end when hovering over a transform is probably from a different bug that has been fixed. (I'd recommend a rebase with trunk to verify).
CleanShot.2021-09-29.at.09.40.04.mp4
tellthemachines
force-pushed
the
add/default-nav-appender
branch
from
12dc8b0 to
4e8457a
Compare
Yup, it's not happening after rebase. |
|
Nice! This heuristic is going to be pretty important going forwards. |
Description
First steps to address #34041:
Changes Navigation block appender to insert Navigation Links by default, unless there are other types of blocks in the Navigation block already.
This is a rough draft so we can have a play and see how it feels.
One thing I'm not sure about is how it should behave when we "Add all pages". Due to the Page List being a different block type, should we show the quick inserter? Or should we treat Page List as if it were just a bunch of Navigation Links?
The other question is how to address submenu: when a Submenu block is added, the quick inserter will appear because Submenus are a different block type. But because they are often part of simple navs, should we consider them the same as Navigation Links?Likewise, it might be good to insert links by default inside submenus too.Updated to insert links by default when submenus are present (and inside submenus).
How has this been tested?
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
This is more of an enhancement than a new feature, but 🤷
Checklist:
*.native.jsfiles for terms that need renaming or removal).