Material-ui: `useAutocomplete` return values wrong on several `getXProps` functions.

Created on 29 Jul 2020  路  12Comments  路  Source: mui-org/material-ui

Definition in question here:
https://github.com/mui-org/material-ui/blob/e294394af1e5b2769ca22c5a084dc3b49933952a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts#L271-L278

I haven't used all of these functions yet, but I know that the following are incorrect:

  • getInputLabelProps
  • getRootProps
  • getInputProps
  • getListboxProps
  • getOptionProps

In my current project I have them typed as the following:

type StringLikeBoolean = boolean | 'false' | 'true';
interface RootProps {
    'aria-owns': string;
    role: string;
    'aria-expanded': StringLikeBoolean
    onClick: () => void;
    onKeyDown: () => void;
    onMouseDown: () => void;
}

interface InputLabelProps {
    htmlFor: string;
    id: string;
}

interface InputProps {
    id: string;
    'aria-activedescendant': string;
    'aria-autocomplete': 'list';
    'aria-controls': string;
    autoCapitalize: 'none';
    autoComplete: 'off';
    onBlur: (event: React.FocusEvent<HTMLInputElement>) => void;
    onChange: (event: React.ChangeEvent<HTMLInputElement>) => void;
    onFocus: (event: React.FocusEvent<HTMLInputElement>) => void;    onMouseDown: () => void;
    ref: React.RefObject<HTMLInputElement>;
    spellCheck: StringLikeBoolean
    value: string;
}

interface OptionProps {
    'aria-disabled': StringLikeBoolean;
    'aria-selected': StringLikeBoolean;
    'data-option-index': number;
    id: string;
    key: number;
    onClick: () => void;
    onMouseOver: () => void;
    onTouchStart: () => void;
    role: 'option';
    tabIndex: number;
}

If these are accurate I would be more than happy to open a pull request.

Autocomplete good first issue typescript

All 12 comments

Just realized the return value has the _true_ definitions. Starting at line 933. Perhaps we could extract these into packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts

https://github.com/mui-org/material-ui/blob/e294394af1e5b2769ca22c5a084dc3b49933952a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.js#L933

@tbremer Thanks for taking the time to report this improvement opportunity.

What do you think of reproducing a similar approach to https://github.com/downshift-js/downshift/blob/f4d446cab26950fd40fc80e4e97f668a0c8adb83/typings/index.d.ts#L112?

That's an interesting idea, and I haven't come across those types from React before.

I think it's safe to assume that the Input will be on an HTMLInputElement, but for less concrete types, such as listbox, root, options, etc. Would it be safe/easiest to use HTMLElement instead?

Yeah I think that outside the <label> and <input> elements we want to specifically target, the other types should be wider.

@tbremer Would you be interested in working on a pull request?

Sorry, work got extremely busy and I don't know when I will have time to make a PR for this.

I think I will have time this week to work on this problem.

I also found something in the onChange handler that should be updated:
https://github.com/mui-org/material-ui/blob/e294394af1e5b2769ca22c5a084dc3b49933952a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts#L242

I had to handle this as the generic HTMLElement type. Is it consistent what element this _should_ be? For instance, my event fires on an li because of how I build my listbox.

@tbremer happy to hear it. Regarding the change event type, shouldn't we follow #21552? https://github.com/DefinitelyTyped/DefinitelyTyped/blob/efd1527d3d4f05beadddc60e3710ed900696813c/types/react/index.d.ts#L1197. The change event can be triggered from a mouse, touch or keyboard event.

AutocompleteClassKey is also not full.
Linter is complaining about the half of the properties used as a class.

type AutocompleteClassKeyExt = | AutocompleteClassKey | 'fullWidth' | 'hasClearIcon' | 'hasPopupIcon' | 'popupIndicatorOpen' | 'inputFocused';

AutocompleteClassKey is also not full.

@ievgennaida Thanks for raising. It looks like the discrepancy between the JavaScript classes implementation and TypeScript definitions will soon be fixed in https://github.com/mui-org/material-ui/pull/22312/files#diff-2089062329bc98c76eaeb4e054ae2d66R68 by @eps1lon.

The following diff seems to do the job:

diff --git a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts
index 7a044f17dc..75819cfd1c 100644
--- a/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts
+++ b/packages/material-ui-lab/src/useAutocomplete/useAutocomplete.d.ts
@@ -288,14 +288,20 @@ export default function useAutocomplete<
 >(
   props: UseAutocompleteProps<T, Multiple, DisableClearable, FreeSolo>
 ): {
-  getRootProps: () => {};
-  getInputProps: () => {};
-  getInputLabelProps: () => {};
-  getClearProps: () => {};
-  getPopupIndicatorProps: () => {};
-  getTagProps: ({ index }: { index: number }) => {};
-  getListboxProps: () => {};
-  getOptionProps: ({ option, index }: { option: T; index: number }) => {};
+  getRootProps: () => React.HTMLAttributes<HTMLDivElement>;
+  getInputProps: () => React.HTMLAttributes<HTMLInputElement>;
+  getInputLabelProps: () => React.HTMLAttributes<HTMLLabelElement>;
+  getClearProps: () => React.HTMLAttributes<HTMLDivElement>;
+  getPopupIndicatorProps: () => React.HTMLAttributes<HTMLDivElement>;
+  getTagProps: ({ index }: { index: number }) => React.HTMLAttributes<HTMLDivElement>;
+  getListboxProps: () => React.HTMLAttributes<HTMLUListElement>;
+  getOptionProps: ({
+    option,
+    index,
+  }: {
+    option: T;
+    index: number;
+  }) => React.HTMLAttributes<HTMLLIElement>;
   id: string;
   inputValue: string;
   value: Value<T, Multiple, DisableClearable, FreeSolo>;

Could I work on this? :)

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ryanflorence picture ryanflorence  路  3Comments

reflog picture reflog  路  3Comments

newoga picture newoga  路  3Comments

mattmiddlesworth picture mattmiddlesworth  路  3Comments

chris-hinds picture chris-hinds  路  3Comments