URLInput: keep the search results label in sync with the results list #45806
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
|
|
|
Size Change: +19 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
Mamaduka
added
[Type] Code Quality
tyxla
approved these changes
Nov 21, 2022
| @@ -81,7 +81,6 @@ const LinkControlSearchInput = forwardRef( | |||
| ...props, | |||
| instanceId, | |||
| withCreateSuggestion, | |||
| currentInputValue: value, | |||
There was a problem hiding this comment.
Sounds like something that needs to be mentioned in the CHANGELOG?
There was a problem hiding this comment.
Yes, I now added info to block-editor about the URLInput.renderSuggestions having a new currentInputValue parameter.
| @@ -60,13 +60,14 @@ class URLInput extends Component { | |||
|
|
|||
| this.suggestionNodes = []; | |||
|
|
|||
| this.isUpdatingSuggestions = false; | |||
| this.suggestionsRequest = null; | |||
There was a problem hiding this comment.
These component instance properties are always a recipe for weird bugs since React doesn't usually observe changes in them. Maybe we should ideally get rid of them all?
There was a problem hiding this comment.
this.suggestionNodes is used to store refs, it can't be in state and needs to stay ref-like.
this.suggestionsRequest is used to prevent state updates from a request if another request has been issued in the meantime. It can't be in state because we need to set it to null on unmount, so that responses arriving after unmount are ignored.
…n block-editor changelog
jsnajdr
force-pushed
the
update/url-input-sync-suggestions
branch
from
458a775
to
4459a77
Compare
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.
Fixes a bug in
URLInputthat I discovered when updating unit tests for the component during the React 18 migration (#45235). When typing into the input field, it displays suggestions either for the empty value ("initial suggestions", list of recently updated pages):or for a non-empty value (the label is visually hidden in production, I made it visible for testing purposes):
The problem is that the label ("Recently updated" or "Search results for") could get out of sync with the list of results. If I change the value from
"act"to"", theURLInput'svalueprop changes immediately, and the label is immediately rerendered from "Search results for 'act'" to "Recently updated", but the results list changes only a while later, after a REST API fetch finishes. That means we're rendering a mis-labeled list in the meantime.It came up when working on a unit test, because the bug makes it impossible to catch the moment when the list get updated. This would be a great way to do it but it doesn't work:
After this patch, it will start working.
There are several other drive-by changes to
URLInput, best reviewed commit by commit:shouldShowInitialSuggestionsmethod runs only on mount, incDM, when the state is guaranteed to have initial values. Therefore we can remove the checks that look atthis.state.onChangehandler doesn't need to callthis.updateSuggestions. ThecDUlifecycle will always do it when thevalueprop changes.valuewhere we don't want to trigger suggestions search. In that case, cancel the outstanding REST request from a previous query, and setstate.suggestionsto[].this.isUpdatingSuggestionsfield can be moved to state, because it is part of state. There was always asetStatecall nearby when I was refactoring: I only needed to add new property to it, never add a new one.How to test:
The
initialSuggestionsfeature is enabled only when editing a Custom Link (core/navigation-link) inside the Navigation (core/navigation) block, so you need to open the site editor and add a block structure like:Then you can play with editing the input and fetching the suggestions.