Skip to content

[material-ui][Autocomplete] Fix Passing slotProps to the TextField #43606

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Sep 4, 2024

@sai6855 sai6855 added bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! labels Sep 4, 2024
@mui-bot
Copy link

mui-bot commented Sep 4, 2024

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 82bb5a4

@sai6855 sai6855 marked this pull request as draft September 4, 2024 10:35
inputLabel: InputLabelPropsProp,
htmlInput: inputPropsProp,
formHelperText: FormHelperTextPropsProp,
select: SelectPropsProp,
...slotProps,
Copy link
Contributor Author

@sai6855 sai6855 Sep 4, 2024

Choose a reason for hiding this comment

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

Issue is props which are set by Autocomplete to input slot or other slots is getting overriden by slotProps object. Instead of overrding i tried to merge inputProps and slotProps.htmlInput and looks like it is working

Additionally we also need to migrate usage of InputProps, InputLabelProps to slotProps in Autocomplete, i think, it can be done in seperate PR

@sai6855 sai6855 marked this pull request as ready for review September 4, 2024 10:52
@sai6855 sai6855 requested a review from DiegoAndai September 4, 2024 10:52
@ZeeshanTamboli ZeeshanTamboli changed the title [Autocomplete] Fix Passing slotProps to the TextField [material-ui][Autocomplete] Fix Passing slotProps to the TextField Sep 6, 2024
@ZeeshanTamboli ZeeshanTamboli added the package: material-ui Specific to @mui/material label Sep 6, 2024
Comment on lines 146 to 149
input:
typeof slotProps.input === 'function'
? slotProps.input
: { ...(InputPropsProp ?? {}), ...(slotProps.input ?? {}) },
Copy link
Member

Choose a reason for hiding this comment

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

Are we doing these checks in other components as well? Do we have some kind of helper for this?

Copy link
Contributor Author

@sai6855 sai6855 Sep 23, 2024

Choose a reason for hiding this comment

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

As far as I know, other components don't have these checks. (Also We don't have any issues reported yet on other components)

Instead of creating new helper, I'm thinking it's better to handle this in useSlotProps hook, so that all slots related logic will be in one place

Copy link
Member

Choose a reason for hiding this comment

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

Thoughts @DiegoAndai?

Copy link
Member

Choose a reason for hiding this comment

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

The useSlot hook already has merging strategies implemented, but I think the problem here is that the Autocomplete should handle the slot props better:

  • Refactor Autocomplete to use slotProps instead of InputProps and inputProps
  • Inside Autocomplete, merge slotProps.input and slotProps.htmlInput
const [HtmlInputSlot, htmlInputProps] = useSlot('htmlInput', {
    elementType: 'input',
    externalForwardedProps, // already defined above
    additionalProps: {
        disabled,
        readOnly,
        ...getInputProps(),
    },
    className: classes.input,
    ownerState,
  });

// ...

// line 730
    {renderInput(
     // ...
     // line 778
    slotProps: {
        htmlInput: htmlInputProps,
    },
},

I haven't tested this, but it should be something similar to the code above.

This will be a complex PR, but in the end, the result should be the slots pattern properly implemented in the Autocomplete component.

We're still discussing if the slots pattern needs changes, cc: @aarongarciah, so this PR might require changes in the future given that decision.

Does that make sense @sai6855?

Copy link
Contributor Author

@sai6855 sai6855 Oct 5, 2024

Choose a reason for hiding this comment

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

@DiegoAndai I'm bit confused with this approch. are you imagining renderInput would look something like this after refactoring?

  {renderInput({
          id,
          disabled,
          fullWidth: true,
          size: size === 'small' ? 'small' : undefined,
-          InputLabelProps: getInputLabelProps(),
-          InputProps: {
-          },
-         inputProps: {
-        }
+     slotProps:{
+     htmlInput: htmlInputProps // derived from useSLot
+    input: inputProps // derived from useSLot
+    inputLable: inputLabelProps // derived from useSlot
+ } 
)}

Assuming this is the approach you want to take, I'm not sure it is possible to merge slotProps from renderInput and slotProps defined in user land without doing any changes in userland.

This is how currently users use Autocomplete, after refactoring params will have slotProps key. If i understand your approch you want slotProps in params key and slotProps in highlighted line to be merged inside Autocomplete. As Autocomplete component don't have any control over slotProps in highlighted line, is it possible to merge without changes to be done in userland?

<Autocomplete
      value="one"
      open
      options={['one', 'two']}
      renderInput={(params) => (
+      <TextField {...params} autoFocus slotProps={{ htmlInput: { className: 'my-class' } }} />
      )}
    />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DiegoAndai what do you think way forward to this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@sai6855 sorry for the late reply.

The path forward for this PR is what I commented here: #43573 (comment). We should update Autocomplete to use slotProps internally to make it consistent but not merge anything on our side. We should also add documentation explaining what I explained in that same comment: "When overriding slotProps.input for the Autocomplete's TextField, params.slotProps.input must be spread"

Does that make sense?

Copy link
Contributor Author

@sai6855 sai6855 Nov 18, 2024

Choose a reason for hiding this comment

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

Got it, before implementing i want to confirm one more thing. If we go with above approach we would be breaking AutocompleteRenderInputParams type, are you comfortable doing this change b/w majors?

export interface AutocompleteRenderInputParams {

Copy link
Member

Choose a reason for hiding this comment

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

If we go with above approach we would be breaking AutocompleteRenderInputParams type

Could we:

  • Add slotProps
  • Keep and deprecate InputProps and inputProps

Internally, the Autocomplete would have to forward InputProps and inputProps to slotProps accordingly.

If that would work, it wouldn't require a breaking change, right?

Copy link
Contributor Author

@sai6855 sai6855 Nov 27, 2024

Choose a reason for hiding this comment

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

Tried adding slotProps here https://github.com/mui/material-ui/pull/43606/files#diff-d626958b0fab135d0026b2cf86b08f733dae8abb98fc406df247b3846dcf8589R788-R794

I'm struggling to figure out how to add slotProps without breaking existing apps.

For example, assume currently Autocomplete is used as below, ref is passed to InputProps (on Line 7) and that ref is passed to TextField, now after this change , TextField doesn't consider InputProps on Line 7 since params will have slotProps.input and nothing gets attached to ref that's passed to InputProps

1      const ref = React.createRef();
2     render(
3        <Autocomplete
4          onOpen={handleOpen}
5         options={['one']}
6          renderInput={(params) => (
7            <TextField {...params} InputProps={{ ...params.InputProps, ref }} />
          )}
        />,
      );

To make above change work, users need to rewrite Autocomplete as below. I'd consider this as breaking change, what do you think, Are you comfortable pushing this change to prod?

1      const ref = React.createRef();
2     render(
3        <Autocomplete
4          onOpen={handleOpen}
5         options={['one']}
6          renderInput={(params) => (
7            <TextField {...params} slotProps={{...slotProps, input:{...slotProps.input, ref}}}  />
          )}
        />,
      );

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 27, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: autocomplete This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
5 participants