-
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
Keep Link UI open upon initial link creation when used in RichText #57726
Keep Link UI open upon initial link creation when used in RichText #57726
Conversation
Here is an example of what occurs if we don't force the re-computation of the popover anchor in Screen.Capture.on.2024-01-10.at.15-17-11.mp4 |
d54866b
to
5959090
Compare
Size Change: +364 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
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.
I think this is a good step in the right direction.
Bugs
|
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.
I say we do a follow-up soon to tidy up this preview in general (#53654). Perhaps something like this: |
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.
This feels much better. A step in the right direction for sure.
@jeryj @richtabor yup I agree with refined previews. I'll merge this after I fix these bugs |
This will also close #53656 |
I feel like this one will be well-received in general and I personally like it a lot, but it might also be seen as adding an extra click. @getdave if I type space or return after adding the link while the modal is open, does that action closes the modal and return the focus to the text cursor? |
…pdate hook" This reverts commit cbcbf41. # Conflicts: # packages/rich-text/README.md # packages/rich-text/src/private-apis.js
b20085c
to
011fe3b
Compare
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.
I think this PR solves a huge UX issue that was just too easy to overcome to be pressing but extremely annoying (closing the link ui immediately after creating the link).
I think Dave addressed all the original issues with the udpates to rich text and alos I don't think the focus trap issue (pre-existing) is blocking this PR.
Thanks for your review @draganescu 👍 Noting that in order to merge this PR we'll need to address the e2e test failures and ensure they aren't failing due to valid regressions. |
…oss editing links
Flaky tests detected in b94bdec. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7628155823
|
|
||
// Deselect the link text by moving the caret to the end of the line | ||
// and the link popover should not be displayed. | ||
await pageUtils.pressKeys( 'End' ); | ||
await expect( linkPopover ).toBeHidden(); |
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.
Should we invert this instead? toBeVisible()
?
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.
You'd need to do a little bit more, since the focus is in a different spot. I modified it to match the spirit of what the test was already testing. fb8f5a9
What?
When a link is initially created within Rich Text, instead of automatically hiding the Link UI, this PR will keep it open to allow further editing of the newly created link (e.g. toggling link settings...etc).
Moreover, when toggling the
Opens in new tab
checkbox on the "Preview" state of the Link UI, the UI will no longer automatically close.Closes #53656
Why?
This allows users to create a link and then immediately edit it's properties without having to go through the process of re-activating the link they just created. This solves a number of issues and is better UX.
How?
This PR has a number of changes which should be carefully assessed:
useAnchor
now exposes aupdate
method on the returnedanchor
object. This is to allow consumers to force the anchor to be updated which may be required under very specific circumstances (such as in this PR).RichTextValue
when the Link UI is submitted, which means the link format remains active, which means that the Link UI continues to be displayed.key
prop on theLinkControl
. This was causing focus loss on initial link creation because the focus target was removed from the DOM when the component was remounted. This is because thekey
prop was computed because on theanchor
returned fromuseAnchor
which (due to the next UX requirements) now changes whilst theLInkControl
component is rendered. I believe this forcable remount has not be necessary since we undertook improvements to the internals ofLinkControl
but I'm pinging @kevin940726 for a confidence check.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-01-10.at.21-20-59.mp4