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 3 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`
youknowriad marked this conversation as resolved.
Show resolved Hide resolved

#### onChange

Function called with the selected value changes.
Expand Down
8 changes: 6 additions & 2 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
28 changes: 28 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,34 @@ 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 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' );

// Pressing Enter/Return selects the currently focused option
await user.keyboard( '{Enter}' );

expect( onChangeSpy ).toHaveBeenCalledTimes( 1 );
expect( onChangeSpy ).toHaveBeenCalledWith( targetOption.value );
expect( input ).toHaveValue( targetOption.label );
} );

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