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

[Autocomplete] Introduce new component #17037

Merged
merged 6 commits into from
Oct 28, 2019
Merged

[Autocomplete] Introduce new component #17037

merged 6 commits into from
Oct 28, 2019

Conversation

brianle1301
Copy link
Contributor

@brianle1301 brianle1301 commented Aug 17, 2019

Lots need to be done in order to provide better customization and API. Feel free to let me know 😄


Edits by @oliviertassinari

Closes #13863
Closes #17414
Closes #10818
Closes #16536
Closes #17322
Closes #17269
Closes #13179
Closes #17775
Closes #2671
Closes #11612

This work is in progress.

Preview https://deploy-preview-17037--material-ui.netlify.com/components/autocomplete/

Progress: ~92/100 hours.

Benchmark

@mui-pr-bot
Copy link

mui-pr-bot commented Aug 17, 2019

@material-ui/core: parsed: +0.11% , gzip: -0.07% 😍
@material-ui/lab: parsed: +17.59% , gzip: +13.41%

Details of bundle changes.

Comparing: e049ad8...569faf6

bundle Size Change Size Gzip Change Gzip
Autocomplete ▲ +125 kB (+Infinity% ) 125 kB ▲ +39.5 kB (+Infinity% ) 39.5 kB
@material-ui/lab ▲ +25.2 kB (+17.59% ) 168 kB ▲ +5.98 kB (+13.41% ) 50.6 kB
useAutocomplete ▲ +11.1 kB (+Infinity% ) 11.1 kB ▲ +4.21 kB (+Infinity% ) 4.21 kB
@material-ui/core ▲ +368 B (+0.11% ) 347 kB ▼ -71 B (-0.07% ) 94.9 kB
@material-ui/core[umd] ▲ +356 B (+0.12% ) 306 kB ▲ +118 B (+0.13% ) 88.2 kB
TextField ▲ +355 B (+0.30% ) 121 kB ▲ +102 B (+0.29% ) 35.3 kB
Select ▲ +326 B (+0.29% ) 112 kB ▲ +104 B (+0.31% ) 33.3 kB
TablePagination ▲ +304 B (+0.22% ) 138 kB ▲ +135 B (+0.33% ) 40.5 kB
NativeSelect ▲ +172 B (+0.23% ) 75 kB ▲ +62 B (+0.26% ) 23.6 kB
Input ▲ +136 B (+0.19% ) 70.6 kB ▲ +44 B (+0.20% ) 22 kB
FilledInput ▲ +136 B (+0.19% ) 71.7 kB ▲ +42 B (+0.19% ) 22.2 kB
OutlinedInput ▲ +136 B (+0.19% ) 72.2 kB ▲ +39 B (+0.17% ) 22.4 kB
InputBase ▲ +135 B (+0.20% ) 68.8 kB ▲ +41 B (+0.19% ) 21.5 kB
FormControl ▲ +32 B (+0.05% ) 62.7 kB ▲ +15 B (+0.08% ) 19.4 kB
IconButton ▼ -29 B (-0.04% ) 74.4 kB ▼ -12 B (-0.05% ) 23.2 kB
Tooltip ▲ +28 B (+0.03% ) 97.6 kB ▼ -3 B (-0.01% ) 30.9 kB
Popper ▲ +23 B (+0.08% ) 28.5 kB ▲ +14 B (+0.14% ) 10.2 kB
ButtonBase ▼ -23 B (-0.03% ) 72.2 kB ▼ -12 B (-0.05% ) 22.6 kB
Tabs ▼ -22 B (-0.03% ) 83.7 kB ▼ -43 B (-0.16% ) 26.6 kB
Radio ▼ -22 B (-0.03% ) 80.9 kB ▼ -36 B (-0.14% ) 25.4 kB
StepButton ▼ -22 B (-0.03% ) 80.5 kB ▼ -32 B (-0.13% ) 25.3 kB
Checkbox ▼ -22 B (-0.03% ) 80 kB ▼ -28 B (-0.11% ) 25.1 kB
Button ▼ -22 B (-0.03% ) 77.7 kB ▼ -22 B (-0.09% ) 24 kB
Switch ▼ -22 B (-0.03% ) 79.3 kB ▼ -21 B (-0.09% ) 24.7 kB
TableSortLabel ▼ -22 B (-0.03% ) 75.6 kB ▼ -21 B (-0.09% ) 23.9 kB
ExpansionPanelSummary ▼ -22 B (-0.03% ) 76.3 kB ▼ -19 B (-0.08% ) 24 kB
BottomNavigationAction ▼ -22 B (-0.03% ) 73.7 kB ▼ -17 B (-0.07% ) 23.3 kB
ToggleButton ▼ -22 B (-0.03% ) 74.4 kB ▼ -14 B (-0.06% ) 23.5 kB
CardActionArea ▼ -22 B (-0.03% ) 73.3 kB ▼ -10 B (-0.04% ) 23.1 kB
Tab ▼ -22 B (-0.03% ) 74.6 kB ▼ -10 B (-0.04% ) 23.6 kB
SpeedDial ▼ -22 B (-0.03% ) 84.3 kB ▼ -3 B (-0.01% ) 26.5 kB
ListItem ▼ -19 B (-0.03% ) 75.4 kB ▼ -10 B (-0.04% ) 23.5 kB
Fab ▼ -18 B (-0.02% ) 75.1 kB ▼ -13 B (-0.06% ) 23.3 kB
MenuItem ▼ -17 B (-0.02% ) 76.4 kB ▼ -12 B (-0.05% ) 23.8 kB
Chip ▲ +11 B (+0.02% ) 69.1 kB ▼ -24 B (-0.11% ) 21.3 kB
docs.main ▼ -9 B (-0.00% ) 599 kB ▲ +4 B (0.00% ) 191 kB
Paper ▼ -4 B (-0.01% ) 60.8 kB ▲ +5 B (+0.03% ) 18.9 kB
Grow ▲ +4 B (+0.02% ) 22.6 kB -- 7.72 kB
Popover ▲ +3 B (0.00% ) 81 kB ▼ -15 B (-0.06% ) 25 kB
Backdrop ▲ +3 B (0.00% ) 66.2 kB ▼ -3 B (-0.01% ) 20.4 kB
FormLabel ▲ +3 B (0.00% ) 61.7 kB ▲ +2 B (+0.01% ) 19.1 kB
MenuList ▲ +3 B (0.00% ) 64.4 kB ▼ -2 B (-0.01% ) 20.1 kB
StepIcon ▲ +3 B (0.00% ) 63.1 kB ▼ -1 B (-0.01% ) 19.6 kB
SvgIcon ▼ -2 B (-0.00% ) 61.5 kB ▲ +2 B (+0.01% ) 19.1 kB
SpeedDialAction ▲ +1 B (0.00% ) 114 kB ▼ -17 B (-0.05% ) 36 kB
Collapse ▲ +1 B (0.00% ) 66.3 kB ▲ +6 B (+0.03% ) 20.5 kB
Typography ▲ +1 B (0.00% ) 62.1 kB ▲ +4 B (+0.02% ) 19.3 kB
List ▲ +1 B (0.00% ) 60.8 kB ▲ +2 B (+0.01% ) 18.9 kB
TreeView -- 64.4 kB ▼ -30 B (-0.15% ) 20.1 kB
SwipeableDrawer -- 89 kB ▼ -29 B (-0.11% ) 27.6 kB
Dialog -- 80.9 kB ▼ -19 B (-0.08% ) 25.1 kB
Drawer -- 82.7 kB ▼ -18 B (-0.07% ) 25.6 kB
Snackbar -- 75.6 kB ▼ -16 B (-0.07% ) 23.5 kB
Grid -- 63.5 kB ▼ -12 B (-0.06% ) 19.9 kB
ListSubheader -- 61.2 kB ▲ +12 B (+0.06% ) 19.2 kB
Modal -- 14.2 kB ▼ -11 B (-0.22% ) 4.96 kB
Badge -- 63.8 kB ▼ -8 B (-0.04% ) 19.7 kB
CardMedia -- 60.8 kB ▲ +8 B (+0.04% ) 19.1 kB
DialogContentText -- 62.5 kB ▼ -8 B (-0.04% ) 19.6 kB
ExpansionPanelActions -- 60.5 kB ▼ -8 B (-0.04% ) 18.9 kB
Zoom -- 22.1 kB ▼ -8 B (-0.11% ) 7.6 kB
Avatar -- 61.1 kB ▲ +7 B (+0.04% ) 19.2 kB
DialogContent -- 60.6 kB ▼ -7 B (-0.04% ) 19 kB
Icon -- 61.2 kB ▲ +7 B (+0.04% ) 19.1 kB
InputAdornment -- 63.5 kB ▲ +7 B (+0.04% ) 20 kB
StepConnector -- 61.1 kB ▲ +7 B (+0.04% ) 19.2 kB
DialogTitle -- 62.7 kB ▼ -6 B (-0.03% ) 19.7 kB
ExpansionPanel -- 69.5 kB ▲ +6 B (+0.03% ) 21.7 kB
FormGroup -- 60.4 kB ▼ -6 B (-0.03% ) 18.9 kB
TableHead -- 60.5 kB ▲ +6 B (+0.03% ) 18.9 kB
TableRow -- 60.9 kB ▲ +6 B (+0.03% ) 19.1 kB
BottomNavigation -- 60.8 kB ▲ +5 B (+0.03% ) 19 kB
CardContent -- 60.4 kB ▲ +5 B (+0.03% ) 18.9 kB
CardHeader -- 63.5 kB ▲ +5 B (+0.03% ) 20 kB
CircularProgress -- 62.5 kB ▲ +5 B (+0.03% ) 19.7 kB
CssBaseline -- 56 kB ▲ +5 B (+0.03% ) 17.5 kB
FormHelperText -- 61.7 kB ▲ +5 B (+0.03% ) 19.3 kB
Hidden -- 64.5 kB ▲ +5 B (+0.02% ) 20.2 kB
InputLabel -- 63.5 kB ▲ +5 B (+0.03% ) 19.8 kB
@material-ui/system -- 14.8 kB ▼ -4 B (-0.10% ) 4.06 kB
Box -- 69.2 kB ▲ +4 B (+0.02% ) 20.9 kB
CardActions -- 60.5 kB ▲ +4 B (+0.02% ) 18.9 kB
DialogActions -- 60.5 kB ▼ -4 B (-0.02% ) 18.9 kB
Divider -- 61 kB ▼ -4 B (-0.02% ) 19.1 kB
GridList -- 60.9 kB ▲ +4 B (+0.02% ) 19.1 kB
Slider -- 73.7 kB ▲ +4 B (+0.02% ) 23.2 kB
ClickAwayListener -- 3.85 kB ▼ -3 B (-0.19% ) 1.55 kB
ListItemAvatar -- 60.5 kB ▲ +3 B (+0.02% ) 18.9 kB
ListItemIcon -- 60.6 kB ▲ +3 B (+0.02% ) 19 kB
Menu -- 86.6 kB ▼ -3 B (-0.01% ) 27.2 kB
Portal -- 2.87 kB ▲ +3 B (+0.23% ) 1.29 kB
RadioGroup -- 61.7 kB ▼ -3 B (-0.02% ) 19.3 kB
SnackbarContent -- 64.2 kB ▲ +3 B (+0.01% ) 20.2 kB
StepLabel -- 67 kB ▲ +3 B (+0.01% ) 21 kB
Stepper -- 63.3 kB ▲ +3 B (+0.02% ) 19.9 kB
TableBody -- 60.5 kB ▲ +3 B (+0.02% ) 18.9 kB
TableFooter -- 60.5 kB ▲ +3 B (+0.02% ) 18.9 kB
TreeItem -- 71.8 kB ▼ -3 B (-0.01% ) 22.6 kB
useMediaQuery -- 2.49 kB ▲ +3 B (+0.29% ) 1.05 kB
ButtonGroup -- 62.6 kB ▼ -2 B (-0.01% ) 19.5 kB
ExpansionPanelDetails -- 60.4 kB ▲ +2 B (+0.01% ) 18.9 kB
FormControlLabel -- 63.9 kB ▼ -2 B (-0.01% ) 20.1 kB
GridListTile -- 62.1 kB ▼ -2 B (-0.01% ) 19.5 kB
GridListTileBar -- 61.6 kB ▼ -2 B (-0.01% ) 19.3 kB
ListItemSecondaryAction -- 60.4 kB ▲ +2 B (+0.01% ) 18.9 kB
ListItemText -- 63.4 kB ▲ +2 B (+0.01% ) 19.9 kB
Rating -- 68.3 kB ▼ -2 B (-0.01% ) 21.8 kB
Slide -- 24.1 kB ▼ -2 B (-0.02% ) 8.21 kB
StepContent -- 67.4 kB ▲ +2 B (+0.01% ) 21 kB
TableCell -- 62.5 kB ▼ -2 B (-0.01% ) 19.6 kB
@material-ui/styles -- 50.8 kB ▼ -1 B (-0.01% ) 15.4 kB
AppBar -- 62.3 kB ▼ -1 B (-0.01% ) 19.5 kB
Breadcrumbs -- 66.4 kB ▼ -1 B (-0.00% ) 20.8 kB
Card -- 61.3 kB ▲ +1 B (+0.01% ) 19.2 kB
Container -- 61.6 kB ▲ +1 B (+0.01% ) 19.2 kB
LinearProgress -- 63.7 kB ▲ +1 B (+0.01% ) 19.8 kB
NoSsr -- 2.19 kB ▼ -1 B (-0.10% ) 1.04 kB
RootRef -- 4.43 kB ▲ +1 B (+0.06% ) 1.67 kB
Skeleton -- 60.9 kB ▲ +1 B (+0.01% ) 19.1 kB
Step -- 61 kB ▼ -1 B (-0.01% ) 19.1 kB
Table -- 61 kB ▲ +1 B (+0.01% ) 19.1 kB
ToggleButtonGroup -- 61.6 kB ▲ +1 B (+0.01% ) 19.4 kB
Toolbar -- 60.7 kB ▲ +1 B (+0.01% ) 19 kB
colorManipulator -- 3.83 kB -- 1.52 kB
docs.landing -- 55.5 kB -- 14.5 kB
Fade -- 22 kB -- 7.6 kB
Link -- 65 kB -- 20.6 kB
MobileStepper -- 66.2 kB -- 20.6 kB
SpeedDialIcon -- 63 kB -- 19.8 kB
styles/createMuiTheme -- 15.2 kB -- 5.36 kB
TextareaAutosize -- 5.06 kB -- 2.11 kB

Generated by 🚫 dangerJS against 569faf6

@mbrookes

This comment has been minimized.

@brianle1301

This comment has been minimized.

@mbrookes

This comment has been minimized.

@Toub

This comment has been minimized.

@brianle1301

This comment has been minimized.

@Toub

This comment has been minimized.

@brianle1301

This comment has been minimized.

@joshwooding

This comment has been minimized.

@oliviertassinari
Copy link
Member

oliviertassinari commented Sep 17, 2019

@dreamsinspace I'm continuing the pull request. They are a lot of different cases to support. I might need two weeks to complete it (> 60 hours), I will let you know if I can find some help :).

@oliviertassinari oliviertassinari changed the title [Autocomplete] Draft work [Autocomplete] Introduce new component Sep 18, 2019
@brianle1301
Copy link
Contributor Author

@oliviertassinari Checked the code and yes it does look like there a lot of cases and options which I wasn't aware of. Thanks for continuing.

@Angelk90
Copy link
Contributor

Hi @dreamsinspace :
I created the following example of a module based on the original input field with multiple selection via chip, in the example I developed the selection is single when the element is selected, the selection chip is shown instead of the single selection chip.
it is also possible to customize it through some props.
I wanted your opinion about it:

codesandbox

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 11, 2019

Alright, it should be ready for a first review pass and hopefully land in the lab, cc @mui-org/core-contributors.

It's not meant to be ready to go to the core. The points I haven't handled:

  • Fix all the bugs, they might be some edge case the logic doesn't handle. it should be fine, we can wait to get reports on them.
  • Write tests. I would like to wait for the API to be more stable.
  • Disabled options and values. I have skipped this problem, we will need to get back to it. solved
  • Provide a headless API, this is related to Provide a version of the components without any styles #6218. The component is quite heavy as it pulls dozens of components, however, the core logic would be valuable to share. We can imagine a Downshift like approach, with a pure hook API. Once this problem is solved, we can fill the 3 empty slots in the documentation (docs for this API, customization example, GitHub label like demo)
  • Provide an API to translate the icon's title
  • I wonder about moving the icon to position absolute
  • unknow?

A few implementation notes:

  • The use of the MenuItem component. I couldn't use it, it's related to Performance issue on multiple select #17806. It was way too slow to render 300 items. I suspect an issue with either our styling solution or the use of too many inheritances (MenuItem > ListItem > ButtonBase > TouchRipple) vs li, it might be x10 slower.
  • I'm not very happy with how the Autocomplete integrates with the TextField, for instance, it loads the Select component for nothing or it has to customize the style to display the chips correctly. Also, the integration is reversed compared to the Select, the select inject a custom Input implementation while the Autocomplete wraps the whole thing.
  • The popup structure is a bit "heavy", maybe we can remove one element, right now, we have Popper (div) > Paper (div) > List (ul).
  • I have done my best from an accessibility point of view, follow WAI-ARIA to the letter.

@Janpot
Copy link
Member

Janpot commented Oct 11, 2019

Haven't had the chance to try it yet, but from the top of your head, would you see any difficulties in trying to make virtualization work together with grouping?

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Whoa looks nice, feels nice. I would take a look at a11y and testing. This would add a somewhat independent view of the API.

I do another review pass on monday. Didn't look much into specific methods in the component.

@@ -143,6 +143,7 @@ const MenuList = React.forwardRef(function MenuList(props, ref) {
const currentFocus = ownerDocument(list).activeElement;

if (key === 'ArrowDown') {
// Prevent scroll of the page
Copy link
Member

Choose a reason for hiding this comment

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

Yes, I'd like to see more of these comments 👍

display: 'flex',
justifyContent: 'flex-start',
alignItems: 'center',
cursor: 'pointer',
Copy link
Member

Choose a reason for hiding this comment

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

This is fine for now since we use it on most actions as well. But we should discuss this at some point. After all pointer cursors are for links not actions.

Though I admit I misused this as well up until recently.

prev.removeAttribute('data-focus');
}

const listboxNode = paperRef.current.querySelector('ul');
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const listboxNode = paperRef.current.querySelector('ul');
const listboxNode = paperRef.current.querySelector('[role="listbox"]');

An unordered list is not necessarily a listbox.

Copy link
Member

@oliviertassinari oliviertassinari Oct 19, 2019

Choose a reason for hiding this comment

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

It breaks with the virtualization demo. react-window makes it a pain to customize the virtualization nodes.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how this relates to react-window. Why does react-window require <ul role="listbox" /> instead of <div role="listbox" />?

Copy link
Member

Choose a reason for hiding this comment

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

Something is not right in the design, diving more.

groupBy,
id: idProp,
includeInputInList = false,
ListComponent = List,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this customization? If we don't have this documented at the moment we should remove it for now.

Copy link
Member

@oliviertassinari oliviertassinari Oct 18, 2019

Choose a reason for hiding this comment

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

It's used for the 10,000 options virtualization demo.

packages/material-ui-lab/src/Autocomplete/Autocomplete.js Outdated Show resolved Hide resolved

popupOpen = freeSolo && filteredOptions.length === 0 ? false : popupOpen;

const focusFocusedValue = useEventCallback(focusedValue2 => {
Copy link
Member

Choose a reason for hiding this comment

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

If its focused we don't need to focus. Could you clarify what kind of input you have and what your're doing?

Copy link
Member

Choose a reason for hiding this comment

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

It's here to handle the focus shift between the input and each tag.

@Janpot
Copy link
Member

Janpot commented Oct 14, 2019

Was testing the gmail send to example.

  1. Seems like the x buttons to remove options don't work.
  2. Another thing I noticed with the multiple values variant is that when it's focused, but the popup is closed (e.g. after adding an option). I'd expect the popup to toggle open again when I click the text area, but it doesn't do anything. Not sure if this is on purpose.

@ccmartell
Copy link

ccmartell commented Oct 16, 2019

If the autocomplete on the lab is up to date, I would personally like to see the following added:

  1. The autocomplete field fills with the 1st value (or highlighted value) on TAB press.
  2. When pressing the down arrow to select an option and then pressing tab, the field would fill out. Currently it returns to the previous value after tabbing when the menu is open.
  3. When you select an option in the list using arrow keys, could a preview be available in the text box?

@oliviertassinari
Copy link
Member

@Janpot Thanks for the feedback

  1. Thanks, I will fix it.
  2. I think that it would prevent to interact with the text input.

@ccmartell Thanks for the feedback

  1. It is available with the autoSelect prop (false by default).
  2. I don't understand the logic of this suggestion.
  3. It is available with the autoComplete prop (false by default).

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 28, 2019

I have done my best within the 100 hours timeframe limit. I don't want to bet more time on it without user feedback. This is definitely not the end of the story for this component. Some of the areas I wish I had time to spend more time on:

  • Tests, there are none. Hopefully, the implementation is buggy enough that people will report bugs and that we will add tests for. @eps1lon has shown interest in looking at this concern. I think that it will be great, it's an opportunity to test the correct WAI-ARIA behavior implementation as well as an opportunity to second guess the implementation.
  • Bundle size: the Autocomplete is about 39.5 kB standalone. I think that it's too much for what's doing. Take the useAutocomplete, it's 4 kB in comparison. Why is it x10 bigger when we style the core logic? The answers are shared among the dependencies. Popper: 10 kB. Does the problem really needs that much to implement the positioning logic? withStyles: 18 kB. styled-components should help to reduce this number.
  • The API. If I had more time, I would have explored an API close to https://sancho-ui.com/components/combobox and https://ui.reach.tech/combobox/. While the single-component approach seems preferable for the audience that builds UIs with us, it's not for the audience that wants to build a design system. I'm wondering if the useAutocomplete is enough for this use case. The hook path seems significantly easier to deploy and use, the future will tell us if we can further optimize.
  • Demos, I wanted to implement this design with the standalone hook API: https://www.behance.net/gallery/27997595/Multi-select-dropdown-tags-field-with-search. Later
  • a11y, I haven't tested it outside VoiceOver, and only in the early phase of the development (0-30 hours).

Thank you everybody for the feedback, x3 more time investment, and we should have something 🌈✨.

@oliviertassinari
Copy link
Member

I'm continuing the pull request. They are a lot of different cases to support. I might need two weeks to complete it (> 60 hours), I will let you know if I can find some help :).

Don't listen to engineers when they estimate how much time it takes to complete a task, 2 weeks turned to 6 weeks 🤷‍♂️.

@dnknitro
Copy link

When it will be released?

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 28, 2019

@dnknitro It's released under v4.0.0-alpha.30. I have seen some crashes, wait for a release or two before being able to use it in production.

@SladeRun14

This comment has been minimized.

@oliviertassinari oliviertassinari added component: autocomplete This is the name of the generic UI component, not the React module! and removed component: Autocomplete legacy labels Oct 30, 2019
@sjsingh85
Copy link
Contributor

@oliviertassinari @dreamsinspace

In the inputbase The onchange handler is not passing in any extra params sent from the consuming component for inputPropsProp.onChange but is sending for onChange prop call.
image

Is this a bug or an expected functionality? May be this is not the right place for this question, but do let me know if I should create a bug for this. Thanks!

@oliviertassinari
Copy link
Member

@sjsingh85 Thanks for the report. Looking at the history of the source, I think that we can try to remove the ...args spread. Do you want to try it?

@sjsingh85
Copy link
Contributor

@oliviertassinari I am sorry but my argument is opposite, we should pass on any args coming in. I faced an issue with my react-select integration after this update as the onChange handler is not able to pass in the 2nd argument which is kind of the meta data the react-select passes along for more control over changes happening inside the control.
See this from react-select doco:
image

I understand this might not be a standard way of doing things but this is how react-select does it.

Do you think passing along other arguments would be a problem? If not I can raise a PR for that.

@oliviertassinari
Copy link
Member

@sjsingh85 Oh ok, I guess that if somebody provides a custom inputComponent, he might expect to be able to listen to non-standard change events. Thanks for the extra details. Do you want to fix this? :)

@sjsingh85
Copy link
Contributor

@oliviertassinari Sure, this will be my first, might take some time for writing appropriate test cases etc.. but hopefully I will put in one soon.

@connorads

This comment has been minimized.

@koistya
Copy link
Contributor

koistya commented Jan 9, 2022

Material UI Autocomplete Playground:

https://stackblitz.com/edit/react-material-ui-typescript?file=src%2Fcombobox.tsx

Pre-configured with Webpack, Babel, TypeScript, Emotion.js, Material UI theme customization including light/dark mode switching.

@sandrina-p
Copy link

Forgive my comment here, but I have a question about the usage of tabindex="-1" in the close button. I asked on Stackoverflow, in case you can answer there.

Thank you :)

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 11, 2022

@sandrina-p from what I recall, the close button is not tabbable because the same outcome can be achieved by emptying the textbox. I wanted the tab behavior to be predictable, regardless of the input having a value or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment