Add Unlink button to LinkControl popover #32541
Merged
Conversation
This file contains hidden or 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
7 tasks
|
Size Change: +249 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
getdave
force-pushed
the
add/unlink-button-to-link-control
branch
from
9bcc0f6 to
0d3c2e3
Compare
Contributor
Author
Contributor
Author
|
@javierarce This is now ready for review. |
kevin940726
reviewed
Jun 29, 2021
kevin940726
reviewed
Jun 29, 2021
packages/block-editor/src/components/link-control/test/index.js
Outdated
Show resolved
Hide resolved
Contributor
|
This works fine, but the hover state has border around the link, and the active state color is blue (with a border, too). I think there shouldn't be any border and those states should use a darker red.
I'm using macOS Big Sur, and tested this branch with Chrome, Firefox, and Safari. Strangely, in Safari the active state is red. |
Contributor
Author
|
@javierarce Thanks for your review 🤝
I'm pretty sure this focus state is there for a11y reasons. Here's another example of it being used on a link:
Aesthetically I'm with you on this, but I'd be concerned that we'd be compromising the experience for users who rely on that focus state to be consistent. Let me know if you think we can still safetly go ahead with this change?
~ I think we could get away with this color change. Let me adjust things.~ I spoke too soon! Having looked into the base CSS rules on the gutenberg/packages/components/src/button/style.scss Lines 174 to 202 in 79e02a2 Also worth noting that the focus style will only show if/when a user uses keyboard to focus the button. Pointer / touch based devices will simply hover and click. The hover state is red. Let me know your thoughts on this. |
Contributor
Author
|
@kevin940726 Are you happy to 👍 this one? It won't be merged until after @javierarce approves Design wise. |
kevin940726
approved these changes
Jun 30, 2021
Member
kevin940726
left a comment
kevin940726
left a comment
There was a problem hiding this comment.
Sure, looks good :). Just a small nitpick.
Contributor
|
@getdave You are totally right about the borders. The color and and the lack of border-radius of the first one looked strange to me and didn't make the connection with the style of the accessible links. And regarding the colors… I agree with you too, let's use the current convention. |
12 tasks
This was referenced Jul 30, 2021
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 #31464 we discovered that the rich preview can occasionally obscure the
Unlinkitem in the block toolbar.As
<Popover>is not aware of the block toolbar, @javierarce suggested adding anUnlinkbutton to the<LinkControl>UI would be a good compromise.Here is the design for the unlink UI provided by @javierarce which is PR implements:
This PR
is a WIP whichimplements that. For reasons of backwards compatibility it does this via the introduction of a newonRemoveproperty which when passed will:Unlinkbutton to be shown in the<LinkControl>'s UI.Unlinkbutton is clicked.As the consumer of
<LinkControl>must ensure that the handler passed asonRemovewill remove the currentvalueof<LinkControl>we have made theonRemoveprop opt-in in order that other uses of<LinkControl>both within and outside of Core will not be effected.However, this PR does add this
Unlinkbehaviour for the format library implementation which means that paragraph and rich text will receive this functionality by default.In addition, the default
onChangehandler for the format library has been updated so that when it is called withnullit treats this as a signal to remove the link.We need to consider:
@wordpress/format-librarywhere it consumes<LinkControl>to ensure it actually works.<LinkControl>will not expectnullas the value of<LinkControl>and thus we may need to make this an opt in feature or fix all the instances in Core we're in control of.How has this been tested?
Unlinkbutton in the bottom corner of the UI.Now check it doesn't appear in other uses of
<LinkControl>within Core. For example the Nav block.Screenshots
Screen.Capture.on.2021-06-29.at.11-39-23.mov
Types of changes
Non breaking change.
Checklist:
*.native.jsfiles for terms that need renaming or removal).