-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable ability to remove a link from the Nav Link block in the Nav Block #33777
Conversation
Size Change: +23 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
Thank you for working on this Dave @getdave ! It is much appreciated! From viewing the above videos this is what I see: There is the Unlink text, but one can not unlink using the link icon, as can be done for instance when removing a link in the Paragraph block. |
I took a look at this and it was relatively trivial to mirror the unlink functionality in the toolbar. However there is one major flaw in this approach within the context of the Nav block. As shown in the video below, with the "unlink" icon in place it is no longer possible to amend an existing link because the link button immediately removes the link rather than toggling the link editing popover. Rather the user must now remove the link entirely and re-add it which I think is a poorer experience. Screen.Capture.on.2021-07-30.at.12-23-41.mp4I did think perhaps the link popover could appear if the link text is selected. However, this would make direct editing the link text more difficult. I think solving this issue is part of a wider problem surrounding the UX of editing
...in such a way that it is simple and yet doesn't break the "direct manipulation" paradigm that is so central to the block editor. Therefore, I'm going to recommend that we leave this PR "as is". As things stand you can now remove a link which is an improvement on the existing situation in Let me know what you think @paaljoachim. If you're happy please do |
The PR looks good as a first step! |
I'm torn on this, I support some of the conclusions in the issue that it shouldn't be possible to unlink a Navigation Link, because it semantically is a link. I'm not sure if it should be so easy for users to add Would be good to get @tellthemachines' input, because #33775 is very closely related, since the use case seems to be creating sub menus (#18866 (comment)). |
Thanks for the PR, thanks for the ping! I'm catching up on some things and did not have a chance to take this for a spin, so I appreciate the GIF! Dan vocalized my initial concern:
However judging from the GIF what this unlink feature does is simply reverting the button to its setup state, which already exists even today: insert a custom URL and don't select a link. That will show the "Add link" text with the wavy underline, indicating it's incomplete. In this state it also won't be published or shown on the frontend. It's possible to create these navigation item draft states in order to create header patterns, and because this PR simply reverts back to that, this seems like an okay solution to me. I do think we could potentially find a better spot for the actual unlink button, perhaps use the unlink icon button next to the "Edit" button here: |
Correct. Sorry I should have made this a lot clearer. The objective is to remove the link and reset the block to its initial state, not to remove the link and allow only text. Indeed, I think @tellthemachines is working on a solution for the "I just want to add a item without making it a link" scenario. I think this PR makes it a lot easier to reset the state of the block if required.
Sure. Happy for a designer to give some input on this. However, it will be a tough one given the limited UI real estate available. |
The state this PR is now:
Seeking 👍 or 👎 reviews. |
I think the popover itself could use so much love (much of it tracked in #30170) that I don't think it really needs to block this PR. 👍 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Description
In #18866 we see that it is impossible to remove a link from the Nav Link block once it's been added. That's not good.
I've already added a
Unlink
feature to the link UI in the format library for rich text and I've been meaning to port it over to the Nav Block link implementation.This PR enables the ability to unset links on the Nav Link block using the
Unlink
button in the link UI.Note: we are intentionally not mirroring this unlink behaviour in the block toolbar for the reasons outlined in this comment.
How has this been tested?
Unlink
button in popover.Nav Link
block should be reset to default state.Screenshots
Post Editor
Screen.Capture.on.2021-07-30.at.10-01-53.mp4
Nav Editor
Screen.Capture.on.2021-07-30.at.10-02-46.mp4
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.js
files for terms that need renaming or removal).