Fix the filtering of the pages on the Parent Page control #55574
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
youknowriad
added
[Type] Bug
|
Size Change: +195 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Oct 24, 2023
There was a problem hiding this comment.
Adding a prop to disable filtering sounds reasonable to me (although I need to reflect a moment on the prop name itself).
An alternative solution could be to allow to specify, for each option, which is the string of text that should be used when doing the filtering (at the moment, it looks like options.label is used)
|
@ciampo for this particular case, we don't really know how the backend filtering works and what fields it uses exactly. We might not even have access to this data. |
ciampo
reviewed
Oct 25, 2023
ciampo
reviewed
Oct 25, 2023
|
I've added a unit tests and some docs. |
|
Flaky tests detected in f2e1230. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6718656043
|
youknowriad
force-pushed
the
fix/page-parent-filtering
branch
from
f56631e
to
d3588d9
Compare
youknowriad
force-pushed
the
fix/page-parent-filtering
branch
from
d3588d9
to
d7e51f5
Compare
youknowriad
force-pushed
the
fix/page-parent-filtering
branch
from
d7e51f5
to
5fc5d70
Compare
ciampo
reviewed
Oct 30, 2023
There was a problem hiding this comment.
I noticed that when shouldFilter={true} the component highlights an option that does not match the text entered.
Kapture.2023-10-30.at.18.21.07.mp4
I wonder if this is related to the getIndexOfMatchingSuggestion going out of sync when the items are re-sorted, as pointed out in this comment.
| } | ||
| } ); | ||
|
|
||
| return startsWithMatch.concat( containsMatch ); | ||
| }, [ inputValue, options ] ); | ||
| return startsWithMatch.concat( containsMatch ).concat( others ); |
There was a problem hiding this comment.
Currently, when using shouldFilter={false} but without providing an alternative list of options, matching items are at the top of the suggested options, while every other item just follows.
I wonder if, instead, we should disable filtering completely when shouldFilter={true} — ie. not looking for a matching suggestion altogether.
I found some relevant examples on the APG website and on the ariakit website showcasing comboboxes with no filtering, where the items order is not affected.
There was a problem hiding this comment.
Personally, I find it interesting that the items where the label match is first and the ones that contain the string come later, and at the end the ones that match for other criterias. So in that sense I do think the ordering is a good thing here. If you're looking for a parent page, you're more likely to select one where the title matches rather than the description.
There was a problem hiding this comment.
I think current behavior is fine personally. Maybe it all comes to the name of the prop, which in fact is about including all options or not.
There was a problem hiding this comment.
I actually like the idea of calling the prop something like showAllOptions, it changes the perspective on what this functionality is really about.
I guess if we keep the ordering, we should try to always match the first item or no item. |
|
Ok I added some code that disable the "select first suggestion" when typing in "shouldFilter" false. I think that's a fine behavior (no suggestions basically) but there's a possible alternative that I wanted to mention: differentiate between explicitly selected suggetions (if you use arrows or something) and the ones the first suggestion that gets automatically selected. In other words, if the user didn't do anything explicitly, we could continue to highlight the first item until the moment he uses arrow navigation in which case we'd keep highlighting his suggestion in that case. |
ciampo
reviewed
Nov 6, 2023
There was a problem hiding this comment.
Left a few comments.
Overall this PR is looking good, but as agreed with @youknowriad , let's wait before merging it, since we're planning on refactoring this component soon to ariakit.
We can always revisit the decision and merge this PR in case the need for this change becomes more urgent.
| } | ||
| } ); | ||
|
|
||
| return startsWithMatch.concat( containsMatch ); | ||
| }, [ inputValue, options ] ); | ||
| return startsWithMatch.concat( containsMatch ).concat( others ); |
There was a problem hiding this comment.
I actually like the idea of calling the prop something like showAllOptions, it changes the perspective on what this functionality is really about.
| }, [ matchingSuggestions, selectedSuggestion ] ); | ||
|
|
||
| // If the current selection isn't present in the list of suggestions, then automatically select the first item from the list of suggestions. | ||
| setSelectedSuggestion( ( previousSuggetion ) => { |
There was a problem hiding this comment.
Nice improvement, using the callback version of the state setter instead of the state variable!
As a tiny nit, there is a typo
Suggested change
| setSelectedSuggestion( ( previousSuggetion ) => { | |
| setSelectedSuggestion( ( previousSuggestion ) => { |
Comment on lines
+246
to
+255
| // Select first item (which should be Japan because the items are ordered) | ||
| await user.keyboard( '{ArrowDown}' ); | ||
|
|
||
| // Pressing Enter/Return selects the currently focused option | ||
| await user.keyboard( '{Enter}' ); | ||
|
|
||
| expect( onChangeSpy ).toHaveBeenCalledTimes( 2 ); | ||
| expect( onChangeSpy ).toHaveBeenCalledWith( targetOption.value ); | ||
| expect( input ).toHaveValue( targetOption.label ); | ||
| } ); |
There was a problem hiding this comment.
What is this part of the test testing for ? It seems like a repeat of the first part of the test.
I think that we should also add a check in this test about all options being shown (which is the main difference when shouldFilter is false)
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?
The Parent Page control uses a REST API to search for pages as you type in the input. In addition to that it uses client side filtering to remove the "options" that don't contain the search term in the "label"/"title". The problem is that the client side filtering removes some valid results that might have the search term in their description for example but not on the title of the pages.
This PR adds a new prop to the ComboboxControl to disable client side filtering and apply it for the parent page selector.
Testing Instructions
1- Add a new page with some text like "one" in its content/description but not in its title and publish it.
2- Create a new page and try to set the "one" page as parent by filtering/typing "one" in the combobox.
3- The "one" page should appear in the results.