Force disable suggestions until URL field is dirty in Link Control #51354
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: +14 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
getdave
commented
Jun 8, 2023
| @@ -126,6 +126,7 @@ const LinkControlSearchInput = forwardRef( | |||
| return ( | |||
| <div className="block-editor-link-control__search-input-container"> | |||
| <URLInput | |||
| disableSuggestions={ currentLink?.url === value } | |||
There was a problem hiding this comment.
value here is the url (only) not the full value object.
getdave
commented
Jun 8, 2023
Comment on lines
-614
to
-619
| // The URL search suggestion should not exist. | ||
| expect( lastSearchResultItem ).not.toHaveTextContent( searchTerm ); | ||
| expect( lastSearchResultItem ).not.toHaveTextContent( 'URL' ); | ||
| expect( lastSearchResultItem ).not.toHaveTextContent( | ||
| 'Press ENTER to add this link' | ||
| ); |
There was a problem hiding this comment.
These assertions were accidentally modified to .not which allowed the test to pass. However, the test itself no longer applies and makes sense because we actually don't want the URL to show up as a fallback anymore. We test against this elsewhere so it's safe to remove this one now.
getdave
commented
Jun 8, 2023
Comment on lines
+959
to
+961
| // Ensure no initial suggestions are shown. | ||
| expect( | ||
| screen.queryByRole( 'listbox', { |
There was a problem hiding this comment.
This diff isn't the best. Basically I'm changing the test to actually assert that no suggestions are shown. Previously we replied on focusing the input to trigger showing the results which had absolutely no merit that I can see.
Now this functionality is being removed anyway so it makes sense to fix and simplify the test.
|
Flaky tests detected in 1b1baef. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5215428476
|
scruffian
approved these changes
Jun 9, 2023
No it's not 🤔 |
|
I spent a lot more time looking into this. The reason for the difference in behaviour in the navigation link component is that it provides a
I think we should merge this and consider if we want to change that behaviour separately. |
|
Thank you very much for looking into this. I really do appreciate it. That said, I'm slightly nervous that we merged this if there's any unusual behaviour still at play. Maybe it's just me 🤷 We will all need to keep a close eye on this. cc @richtabor who might notice changes in behaviour as he's working in/around this component at the moment as well. |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…ordPress#51354) * Force disable suggestions if internal and current value match * Fix broken and outdated tests * Add test coverage
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?
Changes the control to not show suggestions immediately on focusing the input.
Part of #50885
Why?
Currently on
trunkas soon as you focus theURLfield search suggestions are shown. This seems to have been intentional but it is now undesirable (@richtabor to confirm).Instead we only want to show suggestions if the
URLchanges been modified (i.e. it is "dirty").How?
Supplies the
disableSuggestionsprop if the currentvalueobject'surlproperty matches the current internalurlproperty. In this case the field is not dirty and thus we do not want to show suggestions.Testing Instructions
URLfieldTesting Instructions for Keyboard
Screenshots or screencast
Before
Screen.Capture.on.2023-06-08.at.21-18-33.mp4
After
Screen.Capture.on.2023-06-08.at.21-17-39.mp4