-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix FormTokenField rendering #14819
Fix FormTokenField rendering #14819
Conversation
@gziolo I just added a small unit test ensuring that changing the With the code in this branch, the new test passes. However, if one were to change this: gutenberg/packages/components/src/form-token-field/index.js Lines 61 to 64 in ca3e521
to this: const suggestionsDidUpdate = suggestions !== prevProps.suggestions;
// if ( suggestionsDidUpdate || value !== prevProps.value ) {
if ( value !== prevProps.value ) {
this.updateSuggestions( suggestionsDidUpdate );
} the new test fails, which is exactly what we want here. |
Can you include some testing instructions here for a reviewer to know how to verify the intended changes? |
@aduth in essence, this PR updates the rendered suggestions in case a depended-upon prop, A real-life use case is the Tags panel input. Assuming you have a post tag "test". Typing Not sure this helps...? |
That helps clarify, thanks. Including this sort of steps in the initial comment a reviewer knows what to be looking for (like a bug report's "Steps to Reproduce"). |
@aduth @gziolo this is waiting for quite some time now, so I was wondering if there's anything (for me) to do here? 🙂 I hope to have explained the issue, the fix and steps to test/reproduce, and there are some unit tests. Thanks! |
// Make sure to focus the input when the isActive state is true. | ||
if ( this.state.isActive && ! this.input.hasFocus() ) { | ||
this.input.focus(); | ||
} | ||
|
||
const { suggestions, value } = this.props; | ||
const suggestionsDidUpdate = suggestions !== prevProps.suggestions; |
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 probably updates more than you're expecting it to, at least based on the shape of suggestions
being an array, and the array being generated as a new value on each render.
Essentially, it comes down to:
const a = [ 'foo' ];
const b = [ 'foo' ];
console.log( a === b );
// false
Related: http://adripofjavascript.com/blog/drips/object-equality-in-javascript.html
At least in how this is used by FlatTermSelector
, the array is a result of Array#map
, so would be a new value on each render:
const termNames = availableTerms.map( ( term ) => term.name ); |
suggestions={ termNames } |
The map() method creates a new array with the results of calling a provided function on every element in the calling array.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/map
Options:
- Accept the fact that it renders more than we expect it to, but that it might benefit from some reuse if the references stay the same
- Abandon any attempt to avoid updating and always update suggestions
- Consider instead equality of members for sameness (
@wordpress/is-shallow-equal
). There is a small performance overhead to this, if it's worth considering the costliness vs.updateSuggestions
(or whetherupdateSuggestions
must only be called when in-fact it changes)
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.
Hey @aduth - thanks for the feedback, and sorry for the super late reply. I missed this comment (and only read the review one further down). 🤦♂
You are absolutely right, and I now implemented isShallowEqual
to compare suggestions. 👍
Is this PR all good now? (Build and tests are still pasing.) 🙂
Apologies for the delay in revisiting this. There's quite a backlog for reviews. In case you find yourself in a similar situation, please feel welcomed to review other pull requests while waiting to help contribute to a lower review turnaround. |
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.
Thanks for the patience, and for the fix! This looks good to me 👍
Aside: I found it easiest to verify the fix using Chrome's built-in network throttling options.
selectedSuggestionScroll: false, | ||
isExpanded: false, | ||
} ); | ||
this.setState( { incompleteTokenValue: tokenValue }, this.updateSuggestions ); |
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.
Technically we could avoid this.updateSuggestions
as the callback here and consolidate instead to componentDidUpdate
(which can also detect the state change).
* Fix rendering/reselection upon prop changes * Add unit test * Use isShallowEqual to compare suggestions
* Fix rendering/reselection upon prop changes * Add unit test * Use isShallowEqual to compare suggestions
for whatever reason I'm not getting this behavior when importing from but the really strange thing is, if I copy over the source code from |
Hi @sgrund14 , it's been some time since I looked at this pull request, so I can't exactly recall what the problem might be based on what you describe. Regarding the difference from I'd suggest if you suspect there to be a bug in the component, to create a new issue, since it'll be easier to follow-up on than to continue the discussion in this old pull request. |
I'm not referencing the globals, but I'll open up a new issue. Thanks @aduth :) |
Description
The
FormTokenField
component takes both the list of values to select from and the list of currently selected values as props.However, updating and rendering the component only relies on user interaction (i.e., changing what is in the input). Any changes to either the selectable values or the selected values is ignored.
This results in bad UX in case the values are fetched (asynchronously). The behavior is that you would enter something, WordPress goes and fetches the results, passes them to the component, ... and nothing. Rendering will only happen on the next change in the input.
(By the way, this is the exact situation with any non-hierarchical taxonomy input, for example, Tags.)
This PR fixes that logic, and updates the selection if either of the selectable or selected values changes.
How has this been tested?
Steps to reproduce:
A real-life use case is the Tags panel input. Assuming you have a post tag "test". Typing te into the input will kick off an according API call, and you get your response back. But nothing happens. At least not visually. The component has received the updated prop, but only on a third keystroke, say typing "s" to have "tes", you see the result.
Screenshots
Types of changes
Fix logic in rendering/reselection.
Checklist: