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.
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 the filtering of the pages on the Parent Page control #55574
base: trunk
Are you sure you want to change the base?
Fix the filtering of the pages on the Parent Page control #55574
Changes from all commits
8ec15aa
d0ba9ea
5fc5d70
d839e1b
aff1df9
f2e1230
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like the idea of calling the prop something like
showAllOptions
, it changes the perspective on what this functionality is really about.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.
Nice improvement, using the callback version of the state setter instead of the state variable!
As a tiny nit, there is a typo
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.
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
isfalse
)