Fix Link UI when hyperlink has an empty href value
#35043
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
the
[Feature] Link Editing
|
@javierarce I couldn't seem to find the correct icon for the one you used in the mockup in #17972 (comment). I'll happily swap out if you let me know. |
|
Size Change: +276 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
|
@getdave I used the 'alert' icon from the generic list.
|
|
I've tested this branch, and it works great if you manually set the link to an empty string. It also works fine if you click the link icon or use command k and then press enter, but if you just introduce one or more spaces, the interface still shows the empty state. Could we consider avoiding inserting a link if the user presses enter and either the field length is 0 or just contains spaces? |
I was working on this bit in a separate PR but now wondering if it needs fixing here as well. I'll update here as to my progress. |
7 tasks
|
@javierarce I think you're saying you can create an empty link by hitting |
@javierarce Looks like that doesn't exist in the https://github.com/WordPress/gutenberg/tree/trunk/packages/icons/src/library See also https://wordpress.github.io/gutenberg/?path=/story/icons-icon--library. Can I just add a new icon to the package? |
|
Let's use the
|
|
@javierarce Done
|
getdave
force-pushed
the
fix/ux-when-link-ui-has-empty-href
branch
from
f44b8ca to
be3f275
Compare
|
Rebasing to see if that will get the RN tests to go green. |
|
What is the advantage of having an error state over defaulting to an empty link UI?
|
I guess because the format is still applied to the richtext value so you would click on what looks like an active link and then it would show you a UI for adding a link. That might be confusing. I think it's probably better to code in an error state in the component so we're 100% explicit about that. Then if we want to consider link formats with a What do you think? |
I see your point 👍 I wonder how useful these empty links are anyway, e.g. consider how #35060 disallows creating them inside Gutenberg. Would it make sense to do the same for the parser and parse |
Is it worth considering that this might be an error state introduced by accident? For example, what if someone went poking around in the "Code view" and tried their hand at editing HTML by hand and broke the link by removing the href. If we simply remove the link at the parser level, the author will potentially not notice that the link has disappeared. If they do notice and re-create it, they will also have lost any other attributes applied to that link so they'll have to redo all the config. If we show an invalid UI state however, the author will know something went wrong and have a chance to correct it. Moreover, any existing attributes will be preserved. That said, I imagine that is an edge case scenario. The HTML spec says links without |
adamziel
approved these changes
Sep 29, 2021
There was a problem hiding this comment.
All good points @getdave, let's do it!
For example, what if someone went poking around in the "Code view" and tried their hand at editing HTML by hand and broke the link by removing the href.
Yeah accidentally breaking the href isn't too clear in either scenario – chances are I won't click that link even if the formatting is there. Might be worth exploring separately one day.
getdave
force-pushed
the
fix/ux-when-link-ui-has-empty-href
branch
from
be3f275 to
a1e8f94
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #17972 (comment) we learnt that if any inline link (within paragraph text) has an empty
hrefthen the Link UI can end up in an unexpected "preview" state. This UI state provides no feedback to the user as to the cause of the error.This PR resolves that by switching to rendering an "error" state in the link preview if the
value'surlprop is empty.This does not attempt to resolve the situation where you create an empty link using the Link UI. That's handled in a companion PR.
Fixes #17972 (comment)
Why does this happen?
Back when it was created a
forceIsEditingLinkprop was addd to the<LinkControl>in order to force it in and out of "editing" mode.This was then utilised in the "inline link" part of the format library to allow various user UI actions (example) to trigger the Link UI.
However, this allows for an edge case - if
forceIsEditingLinkis set tofalseand thevalue.urlis falsey then:value.urlproperty.Other options considered
I did consider updating the
<LinkControl>to reverse the effect offorceIsEditingLinkif:forceIsEditingLinkisfalse, AND...value.urlis empty.If I had done that then even if
forceIsEditingLinkwasfalsethen the Link UI would not render the preview or the editing mode because of the absence of a validvalue.url.I decided not to pursue this as the prop is very explicitly forcing a state on the LinkControl and therefore I wanted to avoid "magic" within the component subverting that "instruction".
How has this been tested?
We have a new unit test coverage for this scenario.
You can also manually test:
hrefto an empty string.Screenshots
Screen.Capture.on.2021-09-22.at.13-56-56.mp4
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).