[Autocomplete] Introduce new component #17037
Conversation
|
@material-ui/core: parsed: +0.11% , gzip: -0.07% 😍 Details of bundle changes.Comparing: e049ad8...569faf6
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
oliviertassinari
added
component: Autocomplete
new feature
|
@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
changed the title
|
@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. |
|
Hi @dreamsinspace : |
|
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:
A few implementation notes:
|
|
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? |
| @@ -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 | |||
There was a problem hiding this comment.
Yes, I'd like to see more of these comments 👍
| display: 'flex', | ||
| justifyContent: 'flex-start', | ||
| alignItems: 'center', | ||
| cursor: 'pointer', |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
| const listboxNode = paperRef.current.querySelector('ul'); | |
| const listboxNode = paperRef.current.querySelector('[role="listbox"]'); |
An unordered list is not necessarily a listbox.
There was a problem hiding this comment.
It breaks with the virtualization demo. react-window makes it a pain to customize the virtualization nodes.
There was a problem hiding this comment.
I don't understand how this relates to react-window. Why does react-window require <ul role="listbox" /> instead of <div role="listbox" />?
There was a problem hiding this comment.
Something is not right in the design, diving more.
| groupBy, | ||
| id: idProp, | ||
| includeInputInList = false, | ||
| ListComponent = List, |
There was a problem hiding this comment.
Do we need this customization? If we don't have this documented at the moment we should remove it for now.
There was a problem hiding this comment.
It's used for the 10,000 options virtualization demo.
|
|
||
| popupOpen = freeSolo && filteredOptions.length === 0 ? false : popupOpen; | ||
|
|
||
| const focusFocusedValue = useEventCallback(focusedValue2 => { |
There was a problem hiding this comment.
If its focused we don't need to focus. Could you clarify what kind of input you have and what your're doing?
There was a problem hiding this comment.
It's here to handle the focus shift between the input and each tag.
|
Was testing the gmail send to example.
|
|
If the autocomplete on the lab is up to date, I would personally like to see the following added:
|
|
@Janpot Thanks for the feedback
@ccmartell Thanks for the feedback
|
|
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:
Thank you everybody for the feedback, x3 more time investment, and we should have something 🌈✨. |
Don't listen to engineers when they estimate how much time it takes to complete a task, 2 weeks turned to 6 weeks 🤷♂️. |
|
When it will be released? |
|
@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. |
This comment has been minimized.
This comment has been minimized.
oliviertassinari
added
component: autocomplete
|
@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. 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! |
|
@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? |
|
@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. 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. |
|
@sjsingh85 Oh ok, I guess that if somebody provides a custom |
|
@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. |
This comment has been minimized.
This comment has been minimized.
|
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. |
|
Forgive my comment here, but I have a question about the usage of Thank you :) |
|
@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. |
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