Skip to content
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

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Enhancements

- `InputControl`/`SelectControl`: update `height`/`min-height` to `32px` instead of `30px` to align with modern sizing scale ([#55490](https://github.com/WordPress/gutenberg/pull/55490)).
- `ComboboxControl`: Add `shouldFilter` prop to allow the options to be controlled and filtered externally ([#55574](https://github.com/WordPress/gutenberg/pull/55574)).

### Bug Fix

Expand Down
8 changes: 8 additions & 0 deletions packages/components/src/combobox-control/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ Function called when the control's search input value changes. The argument cont
- Type: `( value: string ) => void`
- Required: No

#### shouldFilter

Function called to determine whether the control should filter the list of displayed options based on the user input or whether it's something that is handled externally by updating the `options` prop when the `onFilterValueChange` callback is called.

- Type: `boolean`
- Required: No
- Default: `true`

#### onChange

Function called with the selected value changes.
Expand Down
38 changes: 25 additions & 13 deletions packages/components/src/combobox-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ function ComboboxControl( props: ComboboxControlProps ) {
help,
allowReset = true,
className,
shouldFilter = true,
ciampo marked this conversation as resolved.
Show resolved Hide resolved
messages = {
selected: __( 'Item selected.' ),
},
Expand Down Expand Up @@ -151,18 +152,21 @@ function ComboboxControl( props: ComboboxControlProps ) {
const matchingSuggestions = useMemo( () => {
const startsWithMatch: ComboboxControlOption[] = [];
const containsMatch: ComboboxControlOption[] = [];
const others: ComboboxControlOption[] = [];
const match = normalizeTextString( inputValue );
options.forEach( ( option ) => {
const index = normalizeTextString( option.label ).indexOf( match );
if ( index === 0 ) {
startsWithMatch.push( option );
} else if ( index > 0 ) {
containsMatch.push( option );
} else if ( ! shouldFilter ) {
others.push( option );
}
} );

return startsWithMatch.concat( containsMatch );
}, [ inputValue, options ] );
return startsWithMatch.concat( containsMatch ).concat( others );
Copy link
Contributor

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.

Screenshot 2023-10-30 at 18 22 31

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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.

}, [ shouldFilter, inputValue, options ] );

const onSuggestionSelected = (
newSelectedSuggestion: ComboboxControlOption
Expand Down Expand Up @@ -266,18 +270,26 @@ function ComboboxControl( props: ComboboxControlProps ) {

// Update current selections when the filter input changes.
useEffect( () => {
const hasMatchingSuggestions = matchingSuggestions.length > 0;
const hasSelectedMatchingSuggestions =
getIndexOfMatchingSuggestion(
selectedSuggestion,
matchingSuggestions
) > 0;

if ( hasMatchingSuggestions && ! hasSelectedMatchingSuggestions ) {
// If the current selection isn't present in the list of suggestions, then automatically select the first item from the list of suggestions.
setSelectedSuggestion( matchingSuggestions[ 0 ] );
if ( ! shouldFilter ) {
setSelectedSuggestion( null );
return;
}
}, [ 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 ) => {
Copy link
Contributor

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

Suggested change
setSelectedSuggestion( ( previousSuggetion ) => {
setSelectedSuggestion( ( previousSuggestion ) => {

const hasMatchingSuggestions = matchingSuggestions.length > 0;
const hasSelectedMatchingSuggestions =
getIndexOfMatchingSuggestion(
previousSuggetion,
matchingSuggestions
) > 0;

if ( hasMatchingSuggestions && ! hasSelectedMatchingSuggestions ) {
return matchingSuggestions[ 0 ];
}
return previousSuggetion;
} );
}, [ shouldFilter, matchingSuggestions ] );

// Announcements.
useEffect( () => {
Expand Down
42 changes: 42 additions & 0 deletions packages/components/src/combobox-control/test/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,48 @@ describe.each( [
expect( input ).toHaveValue( targetOption.label );
} );

it( 'should not filter the list of options if shouldFilter is false', async () => {
const user = await userEvent.setup();
const targetOption = timezones[ 0 ];
const japanTargetOption = timezones[ 12 ];
const onChangeSpy = jest.fn();
render(
<Component
options={ timezones }
label={ defaultLabelText }
onChange={ onChangeSpy }
shouldFilter={ false }
/>
);
const input = getInput( defaultLabelText );

// Pressing tab selects the input and shows the options
await user.tab();

// Type enough characters to ensure a predictable search result
await user.keyboard( 'Japan' );

// 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( 1 );
expect( onChangeSpy ).toHaveBeenCalledWith( japanTargetOption.value );
expect( input ).toHaveValue( japanTargetOption.label );

// 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 );
} );
Comment on lines +246 to +255
Copy link
Contributor

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 is false)


it( 'should render aria-live announcement upon selection', async () => {
const user = await userEvent.setup();
const targetOption = timezones[ 9 ];
Expand Down
6 changes: 6 additions & 0 deletions packages/components/src/combobox-control/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,4 +73,10 @@ export type ComboboxControlProps = Pick<
* The current value of the control.
*/
value?: string | null;
/**
* By default the control will filter the options based on the input value.
* but if you're providing filtered options using REST API or something else,
* you may consider disabling the filtering by marking this prop as false.
*/
shouldFilter?: boolean;
};
1 change: 1 addition & 0 deletions packages/editor/src/components/page-attributes/parent.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,7 @@ export function PageAttributesParent() {
options={ parentOptions }
onFilterValueChange={ debounce( handleKeydown, 300 ) }
onChange={ handleChange }
shouldFilter={ false }
/>
);
}
Expand Down
Loading