Link Control require user to manually submit any changes #50668
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
Avoid passing a default when default is handled internally. Set default to be an object as that is what we are tracking.
getdave
commented
May 16, 2023
Comment on lines
418
to
420
| value={ internalControlValue } | ||
| settings={ settings } | ||
| onChange={ onChange } | ||
| onChange={ setInternalSettingValue } |
There was a problem hiding this comment.
This changes means that any changes to settings are stored in internal state and not submitted until the user confirms with "Apply".
getdave
commented
May 16, 2023
Comment on lines
+429
to
+431
| disabled={ | ||
| ! valueHasChanges || currentInputIsEmpty | ||
| } |
There was a problem hiding this comment.
This means the Apply button will not be active until changes to the internal value are detected.
getdave
commented
May 16, 2023
| @@ -2236,7 +2287,7 @@ describe( 'Controlling link title text', () => { | |||
| ).not.toBeInTheDocument(); | |||
| } ); | |||
|
|
|||
| it( 'should reset state on value change', async () => { | |||
| it( 'should reset state upon controlled value change', async () => { | |||
There was a problem hiding this comment.
Unrelated change but it helps improve the meaning of the test.
getdave
commented
May 16, 2023
getdave
commented
May 16, 2023
Comment on lines
+198
to
+199
| // Todo: bug if the missing dep is introduced. Will need a fix. | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
This change is unrelated and is caused by the enabling of the relevant eslint rule. Fixing the code to conform causes a bug so I don't want to fix this now.
getdave
commented
May 16, 2023
Comment on lines
-219
to
+276
| if ( | ||
| currentUrlInputValue !== value?.url || | ||
| internalTextInputValue !== value?.title | ||
| ) { | ||
| if ( valueHasChanges ) { |
There was a problem hiding this comment.
Instead of testing for individual properties of the value, this uses a shallow comparison to check if the external (controlled) and internal values are different. If there is a difference then the internal value has changed and thus we need to submit those changes.
getdave
commented
May 16, 2023
| @@ -241,8 +298,7 @@ function LinkControl( { | |||
| }; | |||
|
|
|||
| const resetInternalValues = () => { | |||
| setInternalUrlInputValue( value?.url ); | |||
| setInternalTextInputValue( value?.title ); | |||
| setInternalControlValue( value ); | |||
There was a problem hiding this comment.
Instead of resetting individual properties, "reset" now just means "sync back to whatever value was/is"
getdave
commented
May 16, 2023
| name: 'Apply', | ||
| } ); | ||
|
|
||
| expect( submitButton ).toBeDisabled(); |
There was a problem hiding this comment.
When there are no changes Submit should be disabled.
getdave
commented
May 16, 2023
Comment on lines
+1818
to
+1822
| await user.click( opensInNewTabToggle ); | ||
|
|
||
| // Check settings are **not** directly submitted | ||
| // which would trigger the onChange handler. | ||
| expect( mockOnChange ).not.toHaveBeenCalled(); |
There was a problem hiding this comment.
Previously changing a setting automatically "submitted" the value (i.e. calls onChange). This asserts that this is no longer the case.
getdave
commented
May 16, 2023
Comment on lines
+1826
to
+1836
| expect( submitButton ).toBeEnabled(); | ||
|
|
||
| // Submit the changed setting value using the Apply button | ||
| await user.click( submitButton ); | ||
|
|
||
| // Assert the value is updated. | ||
| expect( mockOnChange ).toHaveBeenCalledWith( | ||
| expect.objectContaining( { | ||
| opensInNewTab: true, | ||
| } ) | ||
| ); |
There was a problem hiding this comment.
Now a user is required to manually submit any changes to the settings.
|
Size Change: +2.56 kB (0%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
Noting that even if we reject the idea that the |
|
Me again with the same question: do we really need "Apply" and "Cancel" on a document that has almost infinite undo, saves itself and is versioned? Isn't the problem that the UI has some lacking in transmitting what happes? This "cancel or apply" flow makes sense for destructive or complex to undo operations. But for simple toggles it's a bit weird that no other setting from setting intensive UIs such as the inspector require a "commit" action, they just auto commit and hope for the best. E.g. you don't apply increasing padding. You don't apply Bold on a text. It just happens. But, aside from the complaints above, this PR makes things more unitary from an experience point of view so that's good 👏🏻 |
getdave
changed the title
8 tasks
getdave
added
the
[Feature] Link Editing
|
@draganescu Thanks for review. Looks like I'm technically heading in the right direciton.
I'm going to refer you to the revised UX here. I think any discussion about buttons should continue there 🙇 |
|
Ideally I think it would be better to change the toggle to a checkbox, but that should definitely be in a different PR. |
scruffian
reviewed
May 25, 2023
| // Suggestions may contains "settings" values (e.g. `opensInNewTab`) | ||
| // which should not overide any existing settings values set by the | ||
| // user. This filters out any settings values from the suggestion. | ||
| const nonSettingsChanges = Object.keys( updatedValue ).reduce( |
There was a problem hiding this comment.
You have a block of code very similar to this above too. Would moving it to the hook mean that we can combine them?
There was a problem hiding this comment.
Shouldn't require a hook just a function but yes 👍
There was a problem hiding this comment.
Actually in future this code won't be necessary so we can ignore it for now and clean up post-refactor if it's still around.
scruffian
approved these changes
May 25, 2023
I agree. It's going to be done in #50947 |
|
Flaky tests detected in 3e6f682. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5079002929
|
Mamaduka
reviewed
May 30, 2023
Comment on lines
+9
to
+18
| // If the value prop changes, update the internal state. | ||
| useEffect( () => { | ||
| setInternalValue( ( prevValue ) => { | ||
| if ( value && value !== prevValue ) { | ||
| return value; | ||
| } | ||
|
|
||
| return prevValue; | ||
| } ); | ||
| }, [ value ] ); |
There was a problem hiding this comment.
Do you know if this sync hook is still needed? Now that value is an object; the synchronization can cause the content loss.
There was a problem hiding this comment.
So we originally introduced the pattern to fix an issue whereby programmatically updating value didn't update the UI.
@Mamaduka The example is codified in one of the unit tests. Basically if you create x2 separate and different links in a paragraph block and then click between them the UI will reflect the value of that link. Previous to this sync you would continue to see the previous link's value in the UI even if you clicked on another link.
You won't be able to replicate any more because we implemented another fix which passes a key prop to the LinkControl in rich text to force a unique component instance per link instance.
I think we could look to remove the sync. Perhaps a PoC PR is a good place to start. I'll spin up. I've never liked this code and I agree is prone to error.
There was a problem hiding this comment.
Thanks, Dave. Let’s continue discussion in the new issue.
17 tasks
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?
Requires users to manually submit any changes to link "settings" (e.g.
Open in new tab.etc) in the Link UI.Opens in new tabwill not automatically submit the value.Closes #50945
Why?
Since the inception of
<LinkControl>toggling a setting (e.g.Open in new tab) meant that the Link UI automatically submitted itself. This was intended functionality but the implementation had bugs.For example when users made a change to the link URL or Text, and then subsequently toggled the
Open in new tab, the value would get submitted with the change to theopensInNewTabsetting, but without the changes to the URL or Text.This is not the correct behaviour and often resulted in confusion when changes to the URL of Text were not persisted.
We could have changed this so that toggling the toggle submits the entire value including changes to URL and Text, but that seems strange when we have an
Applybutton that implies all changes must be manually submitted. Let's lean on that.This PR changes things so that all changes to settings must be submitted using
Apply. ClickingCancelor otherwise opting out will cause the change to the toggle to be discarded.This UX expectation is reinforced by only enabling the
Applybutton when any part of the value has changed.How?
Rework the tracking of the internal (unsubmitted) values to track the entire
valueobject and not just theurlortitlefields.The purpose of the internal state is to track any as yet unsubmitted changes to the link. This is in order to allow users to discard their changes at any point.
Therefore we must check whether the internal state now differs from the original
valuepassed in. We can safely (and quickly thanks to referential equality) do this with the shallow equality utility as the properties of thevalueare always shallow.With this in place we can selectively track changes to the intenral link value and only submit when the user is ready. Moreover we can improve the UI to only enable
Applywhen changes have been made thereby providing an improved UX.Testing Instructions
Repeat the steps below in the places Link Control is used:
Steps:
Applybutton is not enabled unless you make changes to that link value in some wayTesting Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2023-05-16.at.14-29-54.mp4