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] Improve freeSolo UX #19663

Merged
merged 8 commits into from
Feb 13, 2020

Conversation

itelo
Copy link
Contributor

@itelo itelo commented Feb 12, 2020

This is a draft.

First I started putting all the logic inside the Autocomplete and useAutocomplete, but the more I advance, the more complicated the state was and the code started not looking that good (as you can see here and here). So I made some research and based on react-react created a new component that handles the creation of a new option.

There isn't any tests and no new documentation, first I want to know if It looks good so I can move on.

:)

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

closes #18985

@mui-pr-bot
Copy link

mui-pr-bot commented Feb 12, 2020

No bundle size changes comparing 01b8b09...48cb975

Generated by 🚫 dangerJS against 48cb975

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2020

@itelofilho Why do we need a new component? For instance, what about a new demo? It doesn't seem that we need more than 10 LOCs.

https://codesandbox.io/s/material-demo-bsbxm

/* eslint-disable no-use-before-define */
import React from "react";
import TextField from "@material-ui/core/TextField";
import Autocomplete, {
  createFilterOptions
} from "@material-ui/lab/Autocomplete";

const filter = createFilterOptions();

export default function ComboBox() {
  const [value, setValue] = React.useState(null);

  return (
    <Autocomplete
      value={value}
      onChange={(event, value) => {
        if (value && value.freeSolo) {
          setValue({
            title: value.inputValue
          });
          return;
        }

        setValue(value);
      }}
      filterOptions={(options, params) => {
        const filtered = filter(options, params);

        if (params.inputValue !== "") {
          filtered.push({
            freeSolo: true,
            inputValue: params.inputValue,
            title: `Add "${params.inputValue}"`
          });
        }

        return filtered;
      }}
      id="combo-box-demo"
      options={top100Films}
      getOptionLabel={option => option.title}
      style={{ width: 300 }}
      freeSolo
      renderInput={params => (
        <TextField {...params} label="Combo box" variant="outlined" fullWidth />
      )}
    />
  );
}

// Top 100 films as rated by IMDb users. http://www.imdb.com/chart/top
const top100Films = [
  { title: "The Shawshank Redemption", year: 1994 },
];

@oliviertassinari oliviertassinari added the component: autocomplete This is the name of the generic UI component, not the React module! label Feb 12, 2020
@oliviertassinari oliviertassinari changed the title [autocomplete]: new component to create option in autocomplete [wip] [Autocomplete] Improve freeSolo UX Feb 12, 2020
@oliviertassinari
Copy link
Member

Now, I'm wondering about the level of integration we should give. They are 3 levels:

  1. demos
  2. some way to enable it
  3. default behavior

I have looked at the options react-select provides for it: https://react-select.com/props#creatable-props. It seems that all of them can be easily be implemented with the demo I have proposed. So 1, 2, 3 are all on the table. What would be best? 🤔

What about we go with 1. and look at the demo stats usage? Then, we can better evalute if we should bring it into the core?

@itelo
Copy link
Contributor Author

itelo commented Feb 12, 2020

I agree with you, the 1 is fine for now. Should I close this PR and create a new one with only the new demos or just a rebase is ok?

Also, I believe the most common cases for creating a new option are the two I put in the examples, could I move with it?

@oliviertassinari
Copy link
Member

@itelofilho I think that we could keep iterating on this pull request. The demos look great. For the organization of the documentation, I think that we can put them as h3, after the h2 "Free solo".

On a side note, I'm surprised that Chrome uses a smaller font size (14px?) for its built-in component

Capture d’écran 2020-02-12 à 14 29 26

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 12, 2020
@itelo
Copy link
Contributor Author

itelo commented Feb 13, 2020

I update the examples but in the middle of the process, I found some issues.

  1. When I press the Enter/Return key, the value is cleared. I need to press arrow down and then Enter to the input be filled right.

glitch

  1. When I select the "Add option" sometimes the input text glitches. (the gif is in slow motion)
    slowmo

  2. When I open the Dialog, fill the inputs and close it, the menu is still opened.

dialog

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2020

We are good.

@oliviertassinari
Copy link
Member

I will push the fix for 1 and 2. For 3, if you want to lead the effort, it would be huge :D.

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 13, 2020
@itelo
Copy link
Contributor Author

itelo commented Feb 13, 2020

Awesome! I will take a look at 3.

@oliviertassinari
Copy link
Member

@itelofilho If you could double-check my changes, it would be perfect

@itelo
Copy link
Contributor Author

itelo commented Feb 13, 2020

I just found a little mistake that I already fixed, I believe everything is fine now.

Should I rebase?

@oliviertassinari oliviertassinari merged commit 6ab24fd into mui:master Feb 13, 2020
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 13, 2020

Thanks for the small patch and these new demos :)

@sostenesapollo
Copy link

Nice bro, works like a charm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: autocomplete This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Autocomplete] 'No options' text with freeSolo
4 participants