Remove "preview" mode/step from Link Control component #50998
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
|
Size Change: +18 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
This comment was marked as outdated.
This comment was marked as outdated.
|
It does feel better/more intuitive to edit a link now though. |
|
@richtabor do we still want to pursue this? |
I'm a bit torn. Depends if a version of #53566 and #53654 makes it in. Another idea could be that we plan some link editing preferences (one of which could be removing link previews). I'd like to explore what those could be. |
|
@richtabor Worth noting that recent a11y review of this control suggested that perhaps the preview step isn't that helpful
|
getdave
force-pushed
the
try/link-remove-interstitial-preview-step
branch
from
bd39b55 to
fcf3d74
Compare
|
Flaky tests detected in 39586ff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6109324584
|
getdave
commented
Sep 7, 2023
Comment on lines
+283
to
+305
| // In previous iterations of this component, clicking "Cancel" would toggle | ||
| // the component's UI into "preview" mode. | ||
| // | ||
| // However, this UI state was removed in: | ||
| // https://github.com/WordPress/gutenberg/pull/50998 | ||
| // | ||
| // This poses a backwards compatibility problem as current consumers will | ||
| // assume that the previous UI behaviour is maintained. | ||
| // | ||
| // To avoid this, if a dedicated onCancel hanlder is not provided then the | ||
| // component will simply call onChange with the **original** (unchanged) value. | ||
| // This should effectively save the current value and close the UI. | ||
| if ( onCancel ) { | ||
| onCancel(); | ||
| } else { | ||
| onChange( value ); | ||
| } |
There was a problem hiding this comment.
Here is the current mitigation strategy for backwards compatibility problems.
getdave
added
Backwards Compatibility
getdave
requested review from
ellatrix,
fluiddot,
dcalhoun and
tellthemachines
as code owners
|
Marking this as ready for initial review. Obviously if we proceed with this route then the tests will need updating - assume I will handle that. Other specific points of feedback:
|
getdave
changed the title
getdave
added
Needs Design Feedback
yes!
It feels like a step in the right direction. I find it a bit jarring that the control closes for a couple of reasons. One is that the preview actually loads the URL, so if we keep it open after you have selected it you get some reassurance that your URL is correct. Secondly, keeping the control open gives easier access to edit the link text and advanced settings. That shouldn't stop this from being merged.
I think so as far as I understand it :) |
@scruffian Can you explain what you meant by this?
You're suggesting that the Link Preview might be useful after all? Because we can't keep the control open when you click |
|
Sorry, I mean I find it jarring that the control closes when you add the link for the first time, i.e. when you are creating a link. |
getdave
force-pushed
the
try/link-remove-interstitial-preview-step
branch
from
b7a568e to
a4a0927
Compare
|
I'm flipping this PR back to draft. It's become very complex with too many features packed into a single PR. I'm going to start to break it down into smaller units which we can ship incrementally. I'll list them here as they are raised: |
|
This will not land for WP 6.5 as it needs more time to iterate. |
|
@getdave Could you ping me if you still need this and pair/walk me through the problem? |
|
Guys, sorry to be blunt but it's like you don't even want to listen. Maybe you don't use WordPress for you own personal projects. Maybe you use Joomla. @getdave and @richtabor this is a debate that we have had for a long time now. Since 6.3 I think. In 6.4, the issue was resolved partially. Imagine today the surprise of finding that the open-in-new-tab option has now been rehidden! Now users need to click 4 times for such a simple action. It's frustrating. Why do you mess up things that are working perfectly? Try blogging today and tell me how irritating those clicks become after a short space of time. If things had been originally this way, one would have understood. But things were perfect before 6.3. For some misguided reason, you decided that the open in new tab option, which is used regularly by millions, was not needed. Now we are having this back and forth for almost a year now. You pretend to solve the issue and then you go back to the messed up UI. Why don't you just leave things alone? Maybe you can focus on tables or some other useful areas that need work. Sorry for being caustic but really guys... Bloggers are having far serious issues at the moment with Google's Core update. Why would you foist additional angst on us? Is it because WordPress is free? To think that someone would add a click for the Save button is unfathomable. Anyway, have a good day... |
|
I wonder if we should treat the two cases differently: if there is an internal link chosen from the suggestions box go to preview, if there is a pasted link or an external link typed, go to the details form. I am all for consistency, but in this case external links are different because of reasons, so maybe their different flow is justified? Also, the biggest draw back here is the huge splash of UI when going straight to the details form, but if this is reserved for pasted/external typed links the effect is way smaller in frequency. |
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.
What?
Removes the "Preview" UI state from the Link Control. This means it's always possible to directly edit a link without an extra "edit" click.
Backwards compatbilitybelow.Closes #50892
Why?
As the control has evolved the preview step has become less and less useful. The main concerns are:
LinkControland thus require hacks to force editing when required (e.g.forceIsEditing).By removing the dedicated preview step we solve all these issues whilst improving the usability. The "preview" is still there but it is now alongside the edit state (albeit in a slimed down form).
How?
Backwards compatibility
Previous the
LinkControlhad internal states for edit and preview "modes".Whilst state was internal, this feature was partially exposed in the component API by the addition of the
forceIsEditingprop which was original required to allow richtext formats to trigger the Link UI in "edit" mode when first creating a link in rich text. It's usage has since spread to other parts of Core (and likely is also used by 3rd party consumers).Therefore we need to consider backwards compatibility.
The primary problem is that consumers are used assuming that clicking the
Cancelbutton in the UI will trigger an internal state change in the UI (toggle to "preview" mode) that they have not previously needed to handle. The change in this PR will mean that clickingCancelwould do nothing as there is no longer an preview mode.To mitigate this I have thought of two options:
Call onChange with current
valueobjectWhen the
Cancelbutton is clicked, the component will callonChangewith the currentvalue. As this represents the original value of the control without any modifications, the consuming code should treat this like any other change to the component value. Most consumers treatonChangeas a cue to close the UI and thus using it as a proxy for "cancel" seems logical.If a dedicated
onCancelprop is provided however, we do not callonChangeand assume that the consumer will handle the rendered state (likely visibility within aPopover) of the control.However we are making a number of assumptions.
Hide the Cancel button entirely
In this route, we simply hide the
Cancelbutton unless the consumer supplies a dedicatedonCancelhandler prop. Whilst non-optimal for a UI/UX point of view, this is relatively safe as it makes fewer assumptions.It does however, leave consumers who haven't passed
onCancelwithout a dedicatedCancelbutton.Testing Instructions
Code viewLinkControl.Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-09-07.at.13-08-54.mp4