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

Jump to bottom

[Autocomplete] Improve typings #18854

Merged
merged 10 commits into from
Jan 11, 2020
Merged

[Autocomplete] Improve typings #18854

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d740d65
convert all "any" to generic T
blasterpistol Dec 15, 2019
e603bd7
fix build
oliviertassinari Jan 5, 2020
61b323b
remove types when possible
oliviertassinari Jan 7, 2020
bc56050
use more clear way for variable typings
blasterpistol Jan 8, 2020
266cd42
run proptypes script
blasterpistol Jan 8, 2020
20480c2
remove explicit typings
blasterpistol Jan 10, 2020
34cb52e
run docs:api
blasterpistol Jan 10, 2020
07040cf
add a new limitation
oliviertassinari Jan 10, 2020
1102059
fix spelling
blasterpistol Jan 10, 2020
f66e47b
oops
oliviertassinari Jan 10, 2020
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
12 changes: 6 additions & 6 deletions docs/pages/api/autocomplete.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">closeIcon</span> | <span class="prop-type">node</span> | <span class="prop-default">&lt;CloseIcon fontSize="small" /></span> | The icon to display in place of the default close icon. |
| <span class="prop-name">closeText</span> | <span class="prop-type">string</span> | <span class="prop-default">'Close'</span> | Override the default text for the *close popup* icon button.<br>For localization purposes, you can use the provided [translations](/guides/localization/). |
| <span class="prop-name">debug</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the popup will ignore the blur event if the input if filled. You can inspect the popup markup with your browser tools. Consider this option when you need to customize the component. |
| <span class="prop-name">defaultValue</span> | <span class="prop-type">any</span> | | The default input value. Use when the component is not controlled. |
| <span class="prop-name">defaultValue</span> | <span class="prop-type">any<br>&#124;&nbsp;array</span> | | The default input value. Use when the component is not controlled. |
| <span class="prop-name">disableClearable</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the input can't be cleared. |
| <span class="prop-name">disableCloseOnSelect</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the popup won't close when a value is selected. |
| <span class="prop-name">disabled</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the input will be disabled. |
Expand All @@ -49,7 +49,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">getOptionDisabled</span> | <span class="prop-type">func</span> | | Used to determine the disabled state for a given option. |
| <span class="prop-name">getOptionLabel</span> | <span class="prop-type">func</span> | <span class="prop-default">x => x</span> | Used to determine the string value for a given option. It's used to fill the input (and the list box options if `renderOption` is not provided). |
| <span class="prop-name">getOptionSelected</span> | <span class="prop-type">func</span> | | Used to determine if an option is selected. Uses strict equality by default. |
| <span class="prop-name">groupBy</span> | <span class="prop-type">func</span> | | If provided, the options will be grouped under the returned string. The groupBy value is also used as the text for group headings when `renderGroup` is not provided.<br><br>**Signature:**<br>`function(options: any) => string`<br>*options:* The option to group. |
| <span class="prop-name">groupBy</span> | <span class="prop-type">func</span> | | If provided, the options will be grouped under the returned string. The groupBy value is also used as the text for group headings when `renderGroup` is not provided.<br><br>**Signature:**<br>`function(options: T) => string`<br>*options:* The option to group. |
| <span class="prop-name">id</span> | <span class="prop-type">string</span> | | This prop is used to help implement the accessibility logic. If you don't provide this prop. It falls back to a randomly generated id. |
| <span class="prop-name">includeInputInList</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, the highlight can move to the input. |
| <span class="prop-name">inputValue</span> | <span class="prop-type">string</span> | | The input value. |
Expand All @@ -59,7 +59,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">loadingText</span> | <span class="prop-type">node</span> | <span class="prop-default">'Loading…'</span> | Text to display when in a loading state.<br>For localization purposes, you can use the provided [translations](/guides/localization/). |
| <span class="prop-name">multiple</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, `value` must be an array and the menu will support multiple selections. |
| <span class="prop-name">noOptionsText</span> | <span class="prop-type">node</span> | <span class="prop-default">'No options'</span> | Text to display when there are no options.<br>For localization purposes, you can use the provided [translations](/guides/localization/). |
| <span class="prop-name">onChange</span> | <span class="prop-type">func</span> | | Callback fired when the value changes.<br><br>**Signature:**<br>`function(event: object, value: any) => void`<br>*event:* The event source of the callback.<br>*value:* null |
| <span class="prop-name">onChange</span> | <span class="prop-type">func</span> | | Callback fired when the value changes.<br><br>**Signature:**<br>`function(event: object, value: undefined) => void`<br>*event:* The event source of the callback.<br>*value:* null |
| <span class="prop-name">onClose</span> | <span class="prop-type">func</span> | | Callback fired when the popup requests to be closed. Use in controlled mode (see open).<br><br>**Signature:**<br>`function(event: object) => void`<br>*event:* The event source of the callback. |
| <span class="prop-name">onInputChange</span> | <span class="prop-type">func</span> | | Callback fired when the input value changes.<br><br>**Signature:**<br>`function(event: object, value: string, reason: string) => void`<br>*event:* The event source of the callback.<br>*value:* The new value of the text input.<br>*reason:* Can be: "input" (user input), "reset" (programmatic change), `"clear"`. |
| <span class="prop-name">onOpen</span> | <span class="prop-type">func</span> | | Callback fired when the popup requests to be opened. Use in controlled mode (see open).<br><br>**Signature:**<br>`function(event: object) => void`<br>*event:* The event source of the callback. |
Expand All @@ -71,10 +71,10 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">popupIcon</span> | <span class="prop-type">node</span> | <span class="prop-default">&lt;ArrowDropDownIcon /></span> | The icon to display in place of the default popup icon. |
| <span class="prop-name">renderGroup</span> | <span class="prop-type">func</span> | | Render the group.<br><br>**Signature:**<br>`function(option: any) => ReactNode`<br>*option:* The group to render. |
| <span class="prop-name required">renderInput&nbsp;*</span> | <span class="prop-type">func</span> | | Render the input.<br><br>**Signature:**<br>`function(params: object) => ReactNode`<br>*params:* null |
| <span class="prop-name">renderOption</span> | <span class="prop-type">func</span> | | Render the option, use `getOptionLabel` by default.<br><br>**Signature:**<br>`function(option: any, state: object) => ReactNode`<br>*option:* The option to render.<br>*state:* The state of the component. |
| <span class="prop-name">renderTags</span> | <span class="prop-type">func</span> | | Render the selected value.<br><br>**Signature:**<br>`function(value: any, getTagProps: function) => ReactNode`<br>*value:* The `value` provided to the component.<br>*getTagProps:* A tag props getter. |
| <span class="prop-name">renderOption</span> | <span class="prop-type">func</span> | | Render the option, use `getOptionLabel` by default.<br><br>**Signature:**<br>`function(option: T, state: object) => ReactNode`<br>*option:* The option to render.<br>*state:* The state of the component. |
| <span class="prop-name">renderTags</span> | <span class="prop-type">func</span> | | Render the selected value.<br><br>**Signature:**<br>`function(value: undefined, getTagProps: function) => ReactNode`<br>*value:* The `value` provided to the component.<br>*getTagProps:* A tag props getter. |
| <span class="prop-name">size</span> | <span class="prop-type">'medium'<br>&#124;&nbsp;'small'</span> | <span class="prop-default">'medium'</span> | The size of the autocomplete. |
| <span class="prop-name">value</span> | <span class="prop-type">any</span> | | The value of the autocomplete.<br>The value must have reference equality with the option in order to be selected. You can customize the equality behavior with the `getOptionSelected` prop. |
| <span class="prop-name">value</span> | <span class="prop-type">any<br>&#124;&nbsp;array</span> | | The value of the autocomplete.<br>The value must have reference equality with the option in order to be selected. You can customize the equality behavior with the `getOptionSelected` prop. |

The `ref` is forwarded to the root element.

Expand Down
2 changes: 1 addition & 1 deletion docs/pages/api/bottom-navigation.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">children</span> | <span class="prop-type">node</span> | | The content of the component. |
| <span class="prop-name">classes</span> | <span class="prop-type">object</span> | | Override or extend the styles applied to the component. See [CSS API](#css) below for more details. |
| <span class="prop-name">component</span> | <span class="prop-type">elementType</span> | <span class="prop-default">'div'</span> | The component used for the root node. Either a string to use a DOM element or a component. |
| <span class="prop-name">onChange</span> | <span class="prop-type">func</span> | | Callback fired when the value changes.<br><br>**Signature:**<br>`function(event: object, value: any) => void`<br>*event:* The event source of the callback<br>*value:* We default to the index of the child |
| <span class="prop-name">onChange</span> | <span class="prop-type">func</span> | | Callback fired when the value changes.<br><br>**Signature:**<br>`function(event: object, value: any) => void`<br>*event:* The event source of the callback.<br>*value:* We default to the index of the child. |
| <span class="prop-name">showLabels</span> | <span class="prop-type">bool</span> | <span class="prop-default">false</span> | If `true`, all `BottomNavigationAction`s will show their labels. By default, only the selected `BottomNavigationAction` will show its label. |
| <span class="prop-name">value</span> | <span class="prop-type">any</span> | | The value of the currently selected `BottomNavigationAction`. |

Expand Down
2 changes: 1 addition & 1 deletion docs/pages/api/tree-view.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ You can learn more about the difference by [reading this guide](/guides/minimizi
| <span class="prop-name">defaultExpandIcon</span> | <span class="prop-type">node</span> | | The default icon used to expand the node. |
| <span class="prop-name">defaultParentIcon</span> | <span class="prop-type">node</span> | | The default icon displayed next to a parent node. This is applied to all parent nodes and can be overridden by the TreeItem `icon` prop. |
| <span class="prop-name">expanded</span> | <span class="prop-type">Array&lt;string&gt;</span> | | Expanded node ids. (Controlled) |
| <span class="prop-name">onNodeToggle</span> | <span class="prop-type">func</span> | | Callback fired when tree items are expanded/collapsed.<br><br>**Signature:**<br>`function(event: object, nodeIds: array) => void`<br>*event:* The event source of the callback<br>*nodeIds:* The ids of the expanded nodes. |
| <span class="prop-name">onNodeToggle</span> | <span class="prop-type">func</span> | | Callback fired when tree items are expanded/collapsed.<br><br>**Signature:**<br>`function(event: object, nodeIds: array) => void`<br>*event:* The event source of the callback.<br>*nodeIds:* The ids of the expanded nodes. |

The `ref` is forwarded to the root element.

Expand Down
6 changes: 3 additions & 3 deletions docs/src/pages/components/autocomplete/CheckboxesTags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ export default function CheckboxesTags() {
<Autocomplete
multiple
id="checkboxes-tags-demo"
options={top100Films}
options={top100Films as FilmOptionType[]}
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
disableCloseOnSelect
getOptionLabel={(option: FilmOptionType) => option.title}
renderOption={(option: FilmOptionType, { selected }) => (
getOptionLabel={option => option.title}
renderOption={(option, { selected }) => (
<React.Fragment>
<Checkbox
icon={icon}
Expand Down
4 changes: 2 additions & 2 deletions docs/src/pages/components/autocomplete/ComboBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ export default function ComboBox() {
return (
<Autocomplete
id="combo-box-demo"
options={top100Films}
getOptionLabel={(option: FilmOptionType) => option.title}
options={top100Films as FilmOptionType[]}
getOptionLabel={option => option.title}
style={{ width: 300 }}
renderInput={params => (
<TextField {...params} label="Combo box" variant="outlined" fullWidth />
Expand Down
6 changes: 3 additions & 3 deletions docs/src/pages/components/autocomplete/CountrySelect.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,13 @@ export default function CountrySelect() {
<Autocomplete
id="country-select-demo"
style={{ width: 300 }}
options={countries}
options={countries as CountryType[]}
classes={{
option: classes.option,
}}
autoHighlight
getOptionLabel={(option: CountryType) => option.label}
renderOption={(option: CountryType) => (
getOptionLabel={option => option.label}
renderOption={option => (
<React.Fragment>
<span>{countryToFlag(option.code)}</span>
{option.label} ({option.code}) +{option.phone}
Expand Down
4 changes: 1 addition & 3 deletions docs/src/pages/components/autocomplete/DisabledOptions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export default function DisabledOptions() {
<Autocomplete
id="disabled-options-demo"
options={timeSlots}
getOptionDisabled={(option: TimeSlot) => option === timeSlots[0] || option === timeSlots[2]}
getOptionDisabled={option => option === timeSlots[0] || option === timeSlots[2]}
style={{ width: 300 }}
renderInput={params => (
<TextField {...params} label="Disabled options" variant="outlined" fullWidth />
Expand All @@ -17,8 +17,6 @@ export default function DisabledOptions() {
);
}

type TimeSlot = string;

// One time slot every 30 minutes.
const timeSlots = Array.from(new Array(24 * 2)).map(
(_, index) => `${index < 20 ? '0' : ''}${Math.floor(index / 2)}:${index % 2 === 0 ? '00' : '30'}`,
Expand Down
4 changes: 2 additions & 2 deletions docs/src/pages/components/autocomplete/Filter.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ export default function Filter() {
return (
<Autocomplete
id="filter-demo"
options={top100Films}
getOptionLabel={(option: FilmOptionType) => option.title}
options={top100Films as FilmOptionType[]}
getOptionLabel={option => option.title}
filterOptions={filterOptions}
style={{ width: 300 }}
renderInput={params => (
Expand Down
8 changes: 4 additions & 4 deletions docs/src/pages/components/autocomplete/FixedTags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ export default function FixedTags() {
<Autocomplete
multiple
id="fixed-tags-demo"
options={top100Films}
getOptionLabel={(option: FilmOptionType) => option.title}
options={top100Films as FilmOptionType[]}
getOptionLabel={option => option.title}
defaultValue={[top100Films[6], top100Films[13]]}
renderTags={(value: FilmOptionType[], getTagProps) =>
value.map((option: FilmOptionType, index: number) => (
renderTags={(value, getTagProps) =>
value.map((option, index) => (
<Chip label={option.title} {...getTagProps({ index })} disabled={index === 0} />
))
}
Expand Down
6 changes: 3 additions & 3 deletions docs/src/pages/components/autocomplete/GitHubLabel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ export default function GitHubLabel() {
<span>Labels</span>
<SettingsIcon />
</ButtonBase>
{value.map((label: LabelType) => (
{value.map(label => (
<div
key={label.name}
className={classes.tag}
Expand Down Expand Up @@ -191,7 +191,7 @@ export default function GitHubLabel() {
disablePortal
renderTags={() => null}
noOptionsText="No labels"
renderOption={(option: LabelType, { selected }) => (
renderOption={(option, { selected }) => (
<React.Fragment>
<DoneIcon
className={classes.iconSelected}
Expand All @@ -217,7 +217,7 @@ export default function GitHubLabel() {
bi = bi === -1 ? value.length + labels.indexOf(b) : bi;
return ai - bi;
})}
getOptionLabel={(option: LabelType) => option.name}
getOptionLabel={option => option.name}
renderInput={params => (
<InputBase
ref={params.InputProps.ref}
Expand Down
2 changes: 2 additions & 0 deletions docs/src/pages/components/autocomplete/GoogleMaps.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ const useStyles = makeStyles(theme => ({
}));

interface PlaceType {
description: string;
structured_formatting: {
main_text: string;
secondary_text: string;
main_text_matched_substrings: [
{
Expand Down
8 changes: 5 additions & 3 deletions docs/src/pages/components/autocomplete/Grouped.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,11 @@ export default function Grouped() {
return (
<Autocomplete
id="grouped-demo"
options={options.sort((a, b) => -b.firstLetter.localeCompare(a.firstLetter))}
groupBy={(option: FilmOptionType) => option.firstLetter}
getOptionLabel={(option: FilmOptionType) => option.title}
options={
options.sort((a, b) => -b.firstLetter.localeCompare(a.firstLetter)) as FilmOptionType[]
}
groupBy={option => option.firstLetter}
getOptionLabel={option => option.title}
style={{ width: 300 }}
renderInput={params => (
<TextField {...params} label="With categories" variant="outlined" fullWidth />
Expand Down
4 changes: 2 additions & 2 deletions docs/src/pages/components/autocomplete/Highlights.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ export default function Highlights() {
<Autocomplete
id="highlights-demo"
style={{ width: 300 }}
options={top100Films}
getOptionLabel={(option: FilmOptionType) => option.title}
options={top100Films as FilmOptionType[]}
getOptionLabel={option => option.title}
renderInput={params => (
<TextField {...params} label="Highlights" variant="outlined" fullWidth margin="normal" />
)}
Expand Down
6 changes: 3 additions & 3 deletions docs/src/pages/components/autocomplete/Playground.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import Autocomplete from '@material-ui/lab/Autocomplete';

export default function Playground() {
const defaultProps = {
options: top100Films,
options: top100Films as FilmOptionType[],
getOptionLabel: (option: FilmOptionType) => option.title,
};
const flatProps = {
options: top100Films.map(option => option.title),
};
const [value, setValue] = React.useState(null);
const [value, setValue] = React.useState<FilmOptionType | null>(null);

return (
<div style={{ width: 300 }}>
Expand Down Expand Up @@ -62,7 +62,7 @@ export default function Playground() {
{...defaultProps}
id="controlled-demo"
value={value}
onChange={(event, newValue) => {
onChange={(event: any, newValue: FilmOptionType | null) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Typescript can't discriminate union type without specifying the multiple prop, it's why we need to write concrete type here, an another solution for typescript users is settingmultiple={false} or multiple={undefined} so typescript can infer right typings for onChange handler, what way do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like a bug in typescript microsoft/TypeScript#35769

Copy link
Member

Choose a reason for hiding this comment

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

Great to know. Should we specify the multiple prop (which will be closer to real life usage)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that we can explicitly write multiple={undefined} in typescript examples (with a mention in the documentation why this is needed), and when the bug is fixed, this will not be a breaking change. Or strictly require to specify multiple as true or false, but it seems that this is not a great DX.

Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear, it's only needed because we spread {...defaultProps}? What if we duplicate the props instead?

Copy link
Member

Choose a reason for hiding this comment

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

OK thanks, in this can, I think that it would be better not to change the current tradeoff. The usage of the TypeScript demo is at 10% (and growing). It would look weird to see undefined. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a typescript developer has not defined the multiple prop as false or undefined than all other dependent props will also receive invalid typings, not only the onChange prop.

Copy link
Member

Choose a reason for hiding this comment

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

What about we document it in the ## Limitations section of the page for TypeScript users, with a link to the related TypeScript issue?

Copy link
Contributor Author

@blasterpistol blasterpistol Jan 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sounds good. So we explicitly provide typings for onChange methods, like in this line of code above?

Copy link
Member

Choose a reason for hiding this comment

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

This would be my proposal, I believe we already get significant improvements with your changes. This is a roadblock that will require more effort.

setValue(newValue);
}}
renderInput={params => (
Expand Down
Loading