Navigation: Allow a label to used for placeholder text #35094
Conversation
gwwar
added
[Type] Enhancement
|
Size Change: +181 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
Nice one, this works as intended: One question though, can we enable theme authors to create these patterns in the UI? There's already an "unlink" feature, which perhaps we could tweak so that the label remains? Right now unlinking removes both label and link:
In making unlink just remove the link, arguably it'd be consistent with unlinking text: |
gwwar
force-pushed
the
try/navigation-label-placeholder
branch
from
2f89ee4 to
61519ec
Compare
|
I've updated the unlink function to only remove id/url in 61519ec When we publish, should a link with a label but no url be visible in the published view? CleanShot.2021-09-27.at.09.33.35.mp4 |
Great question. On the one hand my initial instinct was "no". On the other hand, the hope is that the wavy underline/linting indicator of a dead link can scale to blocks like Buttons, where right now you can publish a button that links no-where. I would also like to pair the underline (separate effort) with a pre-publish panel that says "these blocks have dead links", making the wavy underline an extra affordance and not the sole indicator. In that light, maybe it's okay to publish? Not a strong opinion either way, and probably best to choose what feels right and go from there. |
|
@jasmussen If we follow up with a pre-publish check I think it's less of an issue to have this display with the URL empty. That's what current behavior does, but it's harder to get into this state since folks end up needing to edit the markup manually. |
|
@getdave feel free to apply this changeset to the branch 👍 I was hoping that we could maybe land this piece first though to help enable pattern work. |
|
Oh yes don't hold anything up. I just saw the potential synergy in the future 😄 |
|
@jasmussen @getdave I think this piece is relatively safe to land by itself. Do y'all agree or is there any other behavior we should update in this PR? |
|
Looks like we need some input for what should happen in the Navigation Editor if we unlink a subset of properties for a navigation link. In this branch it looks like we point the url at the menu item in that case. CleanShot.2021-09-30.at.12.01.03.mp4Note that serializing link placeholders in cc @getdave @tellthemachines @talldan any thoughts on ideal behavior for the Navigation Editor? |
The Nav Editor should serialize the It looks like it's not possible to Unlink the nav item. If I unlink an item and the reload the Nav editor the unlinked item is restored as though no action occurred. Screen.Capture.on.2021-10-01.at.12-02-06.movLet's look at the block that we're persisting: {
"clientId": "ea02ad66-b946-4c5f-8761-2aa7f7cd541c",
"name": "core/navigation-link",
"isValid": true,
"attributes": {
"label": "",
"type": "",
"id": "",
"opensInNewTab": false,
"url": "",
"kind": "",
"__internalRecordId": 807,
"isTopLevelLink": true
},
"innerBlocks": []
}and here is the block that comes back from the call to gutenberg/packages/edit-navigation/src/store/actions.js Lines 70 to 72 in d079ba2 {
"clientId": "ea02ad66-b946-4c5f-8761-2aa7f7cd541c",
"name": "core/navigation-link",
"isValid": true,
"attributes": {
"label": "my new draft page",
"type": "page",
"kind": "post-type",
"url": "http://localhost:8888/my-new-draft-page/",
"id": 770,
"__internalRecordId": 807
},
"innerBlocks": []
}It looks like perhaps the API ignores the "empty" block and just sends back the original so we can never remove the link. @spacedmonkey Would you have any insight into this? Is the REST API rejecting menu items which don't have any attributes as invalid? |
|
@getdave @gwwar Without looking at it deeply, I would guess it is these lines that might be the issue gutenberg/lib/class-wp-rest-menu-items-controller.php Lines 511 to 519 in 2bbb589 Have you looked in the REST API response? If there a error returned? It is worth noting, that menu items without a title should be removed, like the current menu editor. Screen.Recording.2021-09-11.at.13.39.48.mov |
|
@getdave @spacedmonkey @jasmussen let's explore the Navigation Editor placeholder behavior in a separate PR to address #35271 My current thoughts are that we ideally:
Unless I'm misunderstanding the Navigation Editor project, there's hopefully some wiggle room to adopt new behaviors if it enhances the experience, instead of doing a straightforward functionality port. I can keep this PR on hold until we resolve #35271. |
|
🤔 to unblock this PR I can try to modify unlink behavior depending via a block parameter. Edit: Reading through #30007 it doesn't feel right to serialize an extra block attribute (via block context) to modify behavior that will only be seen in the edit view. Making each editor pass through an unlink function isn't ideal, either since it's easy to forget passing through the needed setting for a new editor, and the functions themselves will likely drift. |
|
Oh maybe if I use something like: |
gwwar
force-pushed
the
try/navigation-label-placeholder
branch
from
61519ec to
871418d
Compare
gwwar
requested review from
adamziel,
draganescu,
kevin940726,
talldan and
tellthemachines
as code owners
|
Okay I think I have this unblocked. In 871418d I disable this new behavior for the navigation editor. cc @getdave @jasmussen much appreciated if folks could take this for another spin |
|
Here's a GIF showing the building of a nice header pattern, leveraging the "unlink" and "keep label" features in this branch:
I think this is potent. I'd love for it to land. I'd defer to Dave on whether it needs an opt-out at all, but if that's what it takes to land this, it seems good to me since there's a clear comment in context, and the amount of code is manageable! Nice work as always. |
There was a problem hiding this comment.
Sorry for the late response. GH comments becoming overwhelming.
I tested the editor and it is correctly disabled.
Screen.Capture.on.2021-10-13.at.14-37-01.mp4
But I have questions about if this is the best way to proceed given the potential upcoming advances to the paradigm around how the Nav block and inner blocks interact.
Specifically I'm concerned about us starting to disable functionality in the editor via props on inner nav blocks.
Why?
As you may have seen the Nav block is possibly moving towards an architecture where the presentation is governed by the wrapping core/navigation block whilst the inner blocks represent the structure of the nav data. Think about the Nav block as being akin to a Template Part and how this facilitates reusable navigation data with different presentations.
The vision is that the Nav Editor becomes another instance of an Isolated Template Part editor, concerned only with editing the inner blocks. In this scenario there would be no wrapping core/navigation block used in the Nav Editor.
One of the benefits of this is that we'd be able to stop having to worry about features added to the Nav block leaking into the Editor and causing regressions. The inner blocks are "raw" so we shouldn't need to disable anything on them.
Therefore (and circling back to my concerns) it we start controlling the inner blocks via props then we're going to be back in the same situation of having to "undo" things on the inner blocks.
Is there a way this feature could be added via the core/navigation block itself - perhaps via some context mechanism. Ideally if the configuring attribute isn't provided to the parent Nav block then the feature shouldn't manifest on the inner block.
I'd like to get a 2nd opinion from @talldan on this one as may disagree.
Edit: forgot to say I ❤️ the feature. Thank you for working on it. It will work very nicely with #33849.
@getdave Thanks for taking another look! I did consider using block context, but this currently only works for block attributes. In this particular case, it didn't feel like the correct fit to serialize a new attribute on the navigation and navigation link, since this is modifying editor behavior only (and its primary use case is for dropping in easier to use patterns). Existing links without the attribute also won't behave as we expect for unsetting a link. Overall, if we're a bit wary about diverging behavior, another option would be fixing behavior in #35271 first. (Though it does sound like there are serialization issues to work through). |
|
For a little bit of context, the TwentyTwentyTwo theme will come with some nice bundled header patterns, including a fresh version of this one. That particular pattern is highly opinionated, and is designed around having precisely 4 menu items, no more, no less. And the pattern is that much more effective when the labels are pre-suggested, knowing a user can always rename them after wiring them up. In that vein, I would really love for a version of this PR to be able to land for 5.9, so such patterns are possible. Given that horizon, is it better to disable it from the nav editor as is done now, or to fix it separately in the nav editor? What's the most feasible? |
Since there already needs to be a cleanup of the For context changes here add the ability to used named placeholders which is very helpful in creating interesting patterns, eg "Home, About, Contact" vs "add link, add link, add link". I'm proposing a fork in behavior here since the Editor Navigation screen doesn't use patterns yet and there are some unsolved inconsistencies with how placeholders are serialized in #35271 |
|
The context I'm missing is why the behavior of 'Unlink' is being changed within the Navigation Block. Is it because that's the only way a pattern builder can build patterns? If so, I don't think that's right, the navigation block should be focusing on the primary use case for the majority of users, not pattern builders. If it's being changed because everyone agrees that the right experience for all users would be for 'Unlink' to leave a label untouched, then it seems like a good change. But I think this is still debatable. For the link variations that are connected to a resource like a page, it might be quite frustrating for the label to remain present after 'Unlink', particularly if that label still matches the page title. Then there's the case of the Navigation Editor, which I don't think should miss out on the best user experience because of past rules. Most of the features disabled in the navigation editor are like that because a theme can't possibly support those features. But this seems like a case that the navigation editor could support. So to summarise, my opinion is that we need to understand how it should work for the majority of users, then implement that everywhere if we can. |
|
To an extent, the navigation block is currently the odd one out. In the cases of every other link, unlinking the item just unlinks it, and does not clear any labels set:
In light of that, it feels odd that the navigation block does it:
The fact that it enables building patterns is a side-effect of adding consistency there. We have spent some time making it easy to build navigation people for most people, lately with the plus instantly inserting a page link, and the URL picker immediately opening to wire up a destination. Creating a menu is currently fairly seamless and smooth. One of the benefits of the navigation block is that it lets you customize it after the fact: change labels if you like, for example prefixing "New:" to an item that's been updated, or to offer general customizability. You can even edit the URL after the fact, should you want to append a query string or fix a typo, or just point it somewhere else entirely. In light of this, the URL dialog is agnostic to the content, and it could be argued that it should work the same everywhere. |
There's quite a strong difference in that those interfaces invite the user to type the label or paragraph text first and then add the link. In that way, linking and unlinking has symmetry. That's not true in the Navigation Link block, the act of linking also creates the label in the majority of cases. |
|
That [the symmetry of linking and unlinking] remains the case, though? This interface is only ever accessible after you've created the link in the first place, at which point you can change both the label and the link to break any symmetry, it feels like the unlink button is just an extension of that. Unlinking feels different to me than removing and reinserting a link block. |
|
It is quite a minor thing, so I don't really feel that strongly about it. It just seems like a really unusual flow that the way to add an unlinked item is:
What use case is this fulfilling for the majority of users? Maybe this will get better with #30170, but I am genuinely confused. |
|
Being such an important part of a website, there's a great deal of value in being able to provide rich patterns as shortcuts to accomplishing good designs. In concert with other features built for this purpose, being able to create an opinionated 4-item pattern like this one can help enable or inspire rich designs.
Patterns, as noted, is a side effect. Being able to unlink an item on its own can be as important as being able to edit a link. Since you can already change labels and URLs after the fact, also keeping the behavior of the unlink button consistent serves to reinforce confidence in the UI. Flipping the question on its head: what use case is fulfilled by keeping the behavior of the unlink button unique to the menu item? |
|
I think the main part I'm concerned about is that 'Unlink' potentially now introduces a system that can be abused by users to add semantically incorrect tags to their websites (anchors without hrefs being used as plain text), something that was previously quite strongly discouraged in the block by the use of the A couple of thoughts:
|
Why? The block itself is called 'link', it seems unusual to consider it valid in an unlinked state. For the issue you linked to, the reporter had a very clear use case, they wanted to create a submenu where the top-level item isn't a link. This has since been solved by the submenu block, which keeps the semantics of the link block in-tact. |
|
Thanks for talking through this with me. As outlined, I believe there's value to be found, but given the concerns voiced, perhaps we should pause this one after all. Of note, the following is possible as a pattern today:
Given that fact gets us 90% there, it could be a good baseline to start from and get some feedback on patterns as they are being created and used. We can then revisit in the future if we find a need to take it further. |
|
Chiming in a bit late, but I don't think we should ever allow a state like this:
Where it's not clear at all if things are in "placeholder" state or there is actual content set. If I have pages that coincide with those names, I might mistake the above as being set already while that's not true. Placeholder states that need the user to interact should be obvious and uncomplicated. |
|
Understood, I might have stared at this for too long. Thanks all, for chiming in and for exploring this one, I'll go ahead and close the adjacent issues. |
|
I think our current solution is not quite right, so we'll need to continue to stare at it but at larger intervals :) |
Changes here allow us to use a label as placeholder text instead of "Select Page". This makes creating navigation patterns more visually interesting and lets users more quickly pick out useful patterns. See related #35063
CleanShot.2021-09-23.at.09.22.39.mp4
Testing Instructions