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

Improve autocomplete types/interface #523

Open
tomaskikutis opened this issue Nov 4, 2020 · 0 comments
Open

Improve autocomplete types/interface #523

tomaskikutis opened this issue Nov 4, 2020 · 0 comments
Assignees
Milestone

Comments

@tomaskikutis
Copy link
Member

Use generics in order to return correct types

onSelect?(suggestion: string): void;

suggestion parameter has incorrect type. In case of auto tagging, it has a type of ITagUi, and not a string.

At the Autocomplete component level, we don't know the types of objects and should use generics in order for types to be resolved correctly:

interface IProps<T> {
    search?(searhString: string, callback: (result: Array<T>) => void): {cancel: () => void};
    onSelect?(suggestion: T): void;
    // ...
}

Replace keyValue with a function

keyValue is typed as a string and won't be type-checked. Also it limits supported data structures, for example, I wouldn't be able to pass a tuple or a map. If we used getValue(item: T): string; it would be type safe and work for any kind of data structure. It should also probably be called getLabel if the result will be displayed in the UI. It's more clear when value is used for IDs which don't appear in the UI and label for things which do.

Better type definition for listItemTemplate

listItemTemplate?(value: any): any; --> listItemTemplate?: React.ComponentType<{item: T}>;

Naming suggestion for search

To keep it clear and consistent, I suggest prefixing functions that get called when a certain event happens with on. Like onClick, onSelect I would name it onSearch

Drop items prop

I would keep props to a minimum so the component is easy to use. It was unclear to me on how I was supposed to use the component since it includes both search and items. Sure, I could try read the docs, but it's even better if we can avoid the confusion in the first place. In case we want synchronous behavior without fetching results from the network, we can still use the same search API:

const items = ['one', 'two', 'three', 'four'];

return (
    <Autocomplete
        search={(searchString, callback) => {
            callback(items.filter((item) => item.includes(searchString)));

            return {
                cancel: () => {},
            };
        }}
    />
);
@pavlovicnemanja pavlovicnemanja self-assigned this Nov 4, 2020
@pavlovicnemanja pavlovicnemanja added this to the v2.1.12 milestone Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants