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.

Sign up for GitHub

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

ComboboxControl: Convert to TypeScript #47581

Merged
merged 18 commits into from
Feb 6, 2023
+296 −382
Merged

ComboboxControl: Convert to TypeScript #47581

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
eef24a2
ComboboxControl: Rename file
mirka Jan 30, 2023
7b22327
Remove tabIndex in BaseControl
mirka Jan 30, 2023
0c0ff12
ComboboxControl: Add types
mirka Jan 30, 2023
2760042
Mark `value` prop as optional
mirka Jan 30, 2023
00f8854
Convert tests
mirka Jan 30, 2023
22e5f4d
Fixup
mirka Jan 30, 2023
bb17a6a
Convert stories
mirka Jan 30, 2023
ae5eb6a
Remove from tsconfig
mirka Jan 30, 2023
497e33f
Add changelog
mirka Jan 30, 2023
98b6471
Add main JSDoc
mirka Jan 30, 2023
8241d6d
Make code snippets more concise
mirka Jan 30, 2023
0365ccb
Add code comment for empty string `value`
mirka Feb 3, 2023
5659821
Show code snippets
mirka Feb 3, 2023
ca5b3b2
Fix bug in Storybook
mirka Feb 3, 2023
550db09
Add explicit default value
mirka Feb 3, 2023
d75b49a
Merge branch 'trunk' into ts-combobox-control
mirka Feb 3, 2023
9d39fbc
Fixup
mirka Feb 6, 2023
91bc89a
Merge branch 'trunk' into ts-combobox-control
mirka Feb 6, 2023
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
([#47384](https://github.com/WordPress/gutenberg/pull/47384)).
- `Button`: Convert to TypeScript ([#46997](https://github.com/WordPress/gutenberg/pull/46997)).
- `QueryControls`: Convert to TypeScript ([#46721](https://github.com/WordPress/gutenberg/pull/46721)).
- `ComboboxControl`: Convert to TypeScript ([#47581](https://github.com/WordPress/gutenberg/pull/47581)).
mirka marked this conversation as resolved.
Show resolved Hide resolved
- `Notice`: refactor to TypeScript ([47118](https://github.com/WordPress/gutenberg/pull/47118)).

### Bug Fix
Expand Down
16 changes: 8 additions & 8 deletions packages/components/src/combobox-control/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,35 +92,35 @@ If this property is added, a help text will be generated using help property as

The options that can be chosen from.

- Type: `Array<{ value: String, label: String }>`
- Type: `Array<{ value: string, label: string }>`
- Required: Yes

#### onFilterValueChange

Function called with the control's search input value changes. The argument contains the next input value.
Function called when the control's search input value changes. The argument contains the next input value.

- Type: `Function`
- Type: `( value: string ) => void`
- Required: No

#### onChange

Function called with the selected value changes.

- Type: `Function`
- Type: `( value: string | null ) => void`
Copy link
Contributor

@ciampo ciampo Feb 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not something strictly for this PR, more of a general reflection to be discussed and prioritised separately)

We should take a look at the type signature of onChange functions across the package. In particular:

  • we should make sure that the value passed as argument of onChange has the same type as the value prop of the component
  • we should standardise the behaviour when the component's value is changed to an empty state (e.g cleared). Should we fire onChange with null as a the argument ? Or undefined ? I believe that for some components, the onChange callback may not be fired at all in similar scenarios
  • we should try to also standardize the shape of the callback's first argument. For example, TokenInputProps['onChange'] passes en object with the value prop, instead of the value directly

What are your thoughts? We could probably open a separate issue to track this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling we won't be able to actually standardize the APIs for our existing components (because back compat). The only thing we can do is agree on a standard moving forward for new components.

• we should make sure that the value passed as argument of onChange has the same type as the value prop of the component
• we should try to also standardize the shape of the callback's first argument

Yes 💯

• we should standardise the behaviour when the component's value is changed to an empty state (e.g cleared)

This will likely not be a concern for new components, since we're moving away from built in reset buttons. If it does arise though, I still think calling onChange( undefined ) is the most straightforward and "standardizable" way. This is an overview of the current built-in reset behavior by the way:

undefined

  • ColorPalette
  • DuotonePicker
  • FontSizePicker
  • GradientPicker
  • RangeControl
  • BoxControl (Record< string, undefined >)

null

  • ComboboxControl
  • DateTimePicker

empty string

  • SearchControl

empty array

  • FormTokenField

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a feeling we won't be able to actually standardize the APIs for our existing components (because back compat). The only thing we can do is agree on a standard moving forward for new components.

Yeah, I have the same suspicion. But as you say, we can at least enforce a standard for future work

This is an overview of the current built-in reset behavior by the way:

This recap is already so valuable, thank you!

mirka marked this conversation as resolved.
Show resolved Hide resolved
- Required: No

#### value

The current value of the input.
The current value of the control.

- Type: `mixed`
- Required: Yes
- Type: `string | null`
- Required: No
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely works with the initial value undefined, as seen in the readme code snippet and in unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting to see how null is being used here, in the context of #47473

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, it might even be the first one I've seen with an explicit null 🤔


#### __experimentalRenderItem

Custom renderer invoked for each option in the suggestion list. The render prop receives as its argument an object containing, under the `item` key, the single option's data (directly from the array of data passed to the `options` prop).

- Type: `Function` - `( args: { item: object } ) => ReactNode`
- Type: `( args: { item: object } ) => ReactNode`
- Required: No

## Related components
Expand Down
59 changes: 41 additions & 18 deletions .../components/src/combobox-control/index.js → ...components/src/combobox-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,23 +30,35 @@ import { FlexBlock, FlexItem } from '../flex';
import withFocusOutside from '../higher-order/with-focus-outside';
import { useControlledValue } from '../utils/hooks';
import { normalizeTextString } from '../utils/strings';
import type { ComboboxControlOption, ComboboxControlProps } from './types';
import type { TokenInputProps } from '../form-token-field/types';

const noop = () => {};

const DetectOutside = withFocusOutside(
class extends Component {
// @ts-expect-error - TODO: Should be resolved when `withFocusOutside` is refactored to TypeScript
handleFocusOutside( event ) {
// @ts-expect-error - TODO: Should be resolved when `withFocusOutside` is refactored to TypeScript
this.props.onFocusOutside( event );
}

render() {
// @ts-expect-error - TODO: Should be resolved when `withFocusOutside` is refactored to TypeScript
return this.props.children;
}
}
);

const getIndexOfMatchingSuggestion = (
selectedSuggestion: ComboboxControlOption | null,
matchingSuggestions: ComboboxControlOption[]
) =>
selectedSuggestion === null
? -1
: matchingSuggestions.indexOf( selectedSuggestion );
Comment on lines +53 to +59
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typesafe helper function to explicitly handle the case when selectedSuggestion is null. Should not change runtime behavior, as Array.indexOf() will return -1 when there is no match.


function ComboboxControl( {
/** Start opting into the new margin-free styles that will become the default in a future version. */
__nextHasNoMarginBottom = false,
__next36pxDefaultSize,
value: valueProp,
Expand All @@ -62,7 +74,7 @@ function ComboboxControl( {
selected: __( 'Item selected.' ),
},
__experimentalRenderItem,
} ) {
}: ComboboxControlProps ) {
const [ value, setValue ] = useControlledValue( {
value: valueProp,
onChange: onChangeProp,
Expand All @@ -80,11 +92,11 @@ function ComboboxControl( {
const [ isExpanded, setIsExpanded ] = useState( false );
const [ inputHasFocus, setInputHasFocus ] = useState( false );
const [ inputValue, setInputValue ] = useState( '' );
const inputContainer = useRef();
const inputContainer = useRef< HTMLInputElement >( null );

const matchingSuggestions = useMemo( () => {
const startsWithMatch = [];
const containsMatch = [];
const startsWithMatch: ComboboxControlOption[] = [];
const containsMatch: ComboboxControlOption[] = [];
const match = normalizeTextString( inputValue );
options.forEach( ( option ) => {
const index = normalizeTextString( option.label ).indexOf( match );
Expand All @@ -98,7 +110,9 @@ function ComboboxControl( {
return startsWithMatch.concat( containsMatch );
}, [ inputValue, options ] );

const onSuggestionSelected = ( newSelectedSuggestion ) => {
const onSuggestionSelected = (
newSelectedSuggestion: ComboboxControlOption
) => {
setValue( newSelectedSuggestion.value );
speak( messages.selected, 'assertive' );
setSelectedSuggestion( newSelectedSuggestion );
Expand All @@ -107,7 +121,10 @@ function ComboboxControl( {
};

const handleArrowNavigation = ( offset = 1 ) => {
const index = matchingSuggestions.indexOf( selectedSuggestion );
const index = getIndexOfMatchingSuggestion(
selectedSuggestion,
matchingSuggestions
);
let nextIndex = index + offset;
if ( nextIndex < 0 ) {
nextIndex = matchingSuggestions.length - 1;
Expand All @@ -118,7 +135,9 @@ function ComboboxControl( {
setIsExpanded( true );
};

const onKeyDown = ( event ) => {
const onKeyDown: React.KeyboardEventHandler< HTMLDivElement > = (
event
) => {
let preventDefault = false;

if (
Expand Down Expand Up @@ -177,7 +196,7 @@ function ComboboxControl( {
setIsExpanded( false );
};

const onInputChange = ( event ) => {
const onInputChange: TokenInputProps[ 'onChange' ] = ( event ) => {
const text = event.value;
setInputValue( text );
onFilterValueChange( text );
Expand All @@ -188,14 +207,17 @@ function ComboboxControl( {

const handleOnReset = () => {
setValue( null );
inputContainer.current.focus();
inputContainer.current?.focus();
};

// Update current selections when the filter input changes.
useEffect( () => {
const hasMatchingSuggestions = matchingSuggestions.length > 0;
const hasSelectedMatchingSuggestions =
matchingSuggestions.indexOf( selectedSuggestion ) > 0;
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.
Expand Down Expand Up @@ -235,15 +257,14 @@ function ComboboxControl( {
className,
'components-combobox-control'
) }
tabIndex="-1"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this tabIndex because it isn't a valid prop on BaseControl and doesn't do anything.

label={ label }
id={ `components-form-token-input-${ instanceId }` }
hideLabelFromVision={ hideLabelFromVision }
help={ help }
>
<div
className="components-combobox-control__suggestions-container"
tabIndex="-1"
tabIndex={ -1 }
onKeyDown={ onKeyDown }
>
<InputWrapperFlex
Expand All @@ -258,8 +279,9 @@ function ComboboxControl( {
onFocus={ onFocus }
onBlur={ onBlur }
isExpanded={ isExpanded }
selectedSuggestionIndex={ matchingSuggestions.indexOf(
selectedSuggestion
selectedSuggestionIndex={ getIndexOfMatchingSuggestion(
selectedSuggestion,
matchingSuggestions
) }
onChange={ onInputChange }
/>
Expand All @@ -279,13 +301,14 @@ function ComboboxControl( {
{ isExpanded && (
<SuggestionsList
instanceId={ instanceId }
match={ { label: inputValue } }
match={ { label: inputValue, value: '' } }
ciampo marked this conversation as resolved.
Show resolved Hide resolved
displayTransform={ ( suggestion ) =>
suggestion.label
}
suggestions={ matchingSuggestions }
selectedIndex={ matchingSuggestions.indexOf(
selectedSuggestion
selectedIndex={ getIndexOfMatchingSuggestion(
selectedSuggestion,
matchingSuggestions
) }
onHover={ setSelectedSuggestion }
onSelect={ onSuggestionSelected }
Expand Down
Loading