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

[material-ui][Autocomplete] Improve design when there's a start adornment for small autocomplete #41781

Merged
merged 35 commits into from
May 9, 2024
Merged

[material-ui][Autocomplete] Improve design when there's a start adornment for small autocomplete #41781

merged 35 commits into from
May 9, 2024

Conversation

TahaRhidouani
Copy link
Contributor

Fix #41780

Changes

Makes the wrap behavior of the autocomplete the same as a textfield.

Before:
image

After:
image

Note: The flex: wrap is still used when the multiple prop is enabled on the autocomplete to wrap the chips over multiple lines.

Prevent the clear icon from colliding over the input text when hovering

Before:
image

After:
image

@mui-bot
Copy link

mui-bot commented Apr 5, 2024

Netlify deploy preview

https://deploy-preview-41781--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b958bea

@TahaRhidouani TahaRhidouani marked this pull request as draft April 5, 2024 17:05
@danilo-leal danilo-leal added package: material-ui Specific to @mui/material component: autocomplete This is the name of the generic UI component, not the React module! labels Apr 5, 2024
@danilo-leal danilo-leal changed the title [material-ui][Autocomplete] Prevent start adornment from causing inpu… [material-ui][Autocomplete] Improve design when there's a start adornment Apr 5, 2024
@danilo-leal danilo-leal added the design This is about UI or UX design, please involve a designer label Apr 5, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@TahaRhidouani Thanks for the pull request. Left some questions above.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good mostly. Left few comments.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

@TahaRhidouani I've made some tweaks in the theme overrides test, and the implementation we have here resolves the issue. Thanks for your contribution.

However, now that we aim to support CSS extraction, I'm uncertain about the correct and preferred approach. The current method, introducing a new multiple class, will be compatible with CSS extraction when Pigment CSS is used, as we haven't utilized ownerState here but increases bundle size slightly.

I'm unsure about the definitive approach.

This discussion pertains specifically to the flex-wrap style when Autocomplete has the multiple prop.

Here are some points to consider:

  • We didn't employ ownerState in this case. The conditional application of styles using ownerState was eliminated in [Autocomplete] Convert to support CSS extraction #40330, and theme variants were employed to facilitate CSS extraction. However, the advantage of utilizing ownerState is that it eliminates the need for introducing a new class.
  • In this PR, the application of the flex-wrap style when multiple is true introduced a new multiple class. This may lead to increased HTML bloating and a slight increase in bundle size for the already hefty Autocomplete component. Nevertheless, this approach will support CSS extraction using Pigment CSS.
  • We could utilize theme variants and apply the flex-wrap style when multiple is true as done in [Autocomplete] Convert to support CSS extraction #40330. This eliminates the need for introducing a new multiple class and also supports CSS extraction. However, there's a downside to overriding via theme as shown below.

Regarding point 3, with the current solution developers could directly do (as shown in the test):

const theme = createTheme({
  components: {
    MuiAutocomplete: {
      styleOverrides: {
        multiple: {
          padding: '15px',
        },
      },
    },
  },
});

With theme variants developers will have to target the inputRoot class:

const theme = createTheme({
  components: {
    MuiAutocomplete: {
      variants: [
        {
          props: { multiple: true },
          style: {
            [`& .${autocompleteClasses.inputRoot}`]: {
              flexWrap: 'no-wrap',
            },
          },
        },
      ],
    },
  },
});

Developers need to be educated to target inputRoot here whereas in styleOverrides above they can directly target multiple class because we resolve it in overridesResolver:

{
  [`& .${autocompleteClasses.inputRoot}`]: {
    [`&.${autocompleteClasses.multiple}`]: multiple && styles.multiple,
  },
},

So, what's the optimal choice here? @DiegoAndai

@DiegoAndai
Copy link
Member

DiegoAndai commented May 6, 2024

Hey @ZeeshanTamboli and @TahaRhidouani!

So, what's the optimal choice here?

I'm not against a multiple class, but it should be applied to the root and not inputRoot. Applying it to inputRoot is helpful for this use case, but it would be inconsistent. So, if we were to use classes, the correct styleOverrides usage would be

const theme = createTheme({
  components: {
    MuiAutocomplete: {
      styleOverrides: {
        multiple: {
          [`& .${autocompleteClasses.inputRoot}`]: {
              flexWrap: 'no-wrap',
            },
        },
      },
    },
  },
});

Which is similar to what we would have by using a theme variant.

So, to answer the question, both using classes or theme variants are valid options.

The ideal path IMO is to use theme variants in this PR, and have a separate PR adding the multiple class.

@ZeeshanTamboli
Copy link
Member

@DiegoAndai Since this PR has been open for a while, I made the changes myself to use variants. I've also included a visual regression test - https://app.argos-ci.com/mui/material-ui/builds/27693/89392040. Could you review?

Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @TahaRhidouani and @ZeeshanTamboli

@ZeeshanTamboli ZeeshanTamboli added the needs cherry-pick The PR should be cherry-picked to master after merge label May 9, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [material-ui][Autocomplete] Improve design when there's a start adornment [material-ui][Autocomplete] Improve design when there's a start adornment for small autocomplete May 9, 2024
@ZeeshanTamboli ZeeshanTamboli merged commit 5820e1d into mui:next May 9, 2024
19 checks passed
github-actions bot pushed a commit that referenced this pull request May 9, 2024
…ment for small autocomplete (#41781)

Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
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! design This is about UI or UX design, please involve a designer needs cherry-pick The PR should be cherry-picked to master after merge package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[material-ui][Autocomplete] Adding start adornment on small autocompletes causes text to wrap
10 participants