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

[DataGrid] Refactor: create TextField props #16174

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Jan 14, 2025

Part of the design-system agnostic work.

Create props for base TextField.

@romgrk romgrk added the component: data grid This is the name of the generic UI component, not the React module! label Jan 14, 2025
@mui-bot
Copy link

mui-bot commented Jan 14, 2025

Deploy preview: https://deploy-preview-16174--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against b2421e6

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2025
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 16, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Comment on lines 7 to 33
export type GridFilterInputValueProps<Api extends GridApiCommon = GridApiCommunity> = {
item: GridFilterItem;
applyValue: (value: GridFilterItem) => void;
// Is any because if typed as GridApiRef a dep cycle occurs. Same happens if ApiContext is used.
apiRef: React.RefObject<Api>;
inputRef?: React.Ref<any>;
focusElementRef?: React.Ref<any>;
clearButton?: React.ReactNode | null;
/**
* It is `true` if the filter either has a value or an operator with no value
* required is selected (for example `isEmpty`)
*/
isFilterActive?: boolean;
onFocus?: React.FocusEventHandler;
onBlur?: React.FocusEventHandler;
} & Pick<
TextFieldProps,
| 'color'
| 'error'
| 'helperText'
| 'size'
| 'variant'
| 'disabled'
| 'label'
| 'placeholder'
| 'tabIndex'
>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm thinking about making a breaking change and splitting the textfield props in a separate props object than the GridFilterInputValueProps one, because I feel like having the filter props plus the textfield props merged into a single object is messy. It also makes it harder to parametrize the filter props type in case we want to allow other design-systems to augment the base textfield props. Also, some filters aren't textfields.

Copy link
Member

Choose a reason for hiding this comment

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

splitting the textfield props in a separate props object than the GridFilterInputValueProps one

Do you mean nesting textfield props in GridFilterInputValueProps?

type GridFilterInputValueProps = {
  item: GridFilterItem;
  // ...
  textFieldProps: Pick<TextFieldProps, 'color' /* | ... */>,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, though that component is the base type for all other filters' .InputComponent (I'm thinking about renaming it GridFilterInputComponentProps), and I don't see that we're using all those base textfield props in practice, so I'm thinking something like this:

type GridFilterInputComponentProps<T> = {
  item: GridFilterItem;
  // ...
  slotProps: {
    component: T
  },
}

And to lift any prop that's actually used into the root GridFilterInputComponentProps object.

I want to avoid allowing customizing the base input component via props on the wrapper, because other design systems might have base slots that accept more than the base props (e.g. to allow customization through slotProps), and I don't want name conflicts between an .InputComponent wrapper and its base input component.

I'll do the same for the GridToolbarQuickFilter that also exposes textfield props on itself.

Copy link
Contributor Author

@romgrk romgrk Jan 17, 2025

Choose a reason for hiding this comment

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

(e.g. to allow customization through slotProps)

And the background for that is that I'm thinking about all the fine-grained customization even we are going to need to do, e.g. adding shrink on some textfields or disabling the ripple on some components. We can't expose props for that in our base typings but we need a way to keep doing those small changes, so I think adding slotProps everywhere that's needed is the best & most consistent way to do it so it works across design-systems.

Copy link
Member

Choose a reason for hiding this comment

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

so I think adding slotProps everywhere that's needed is the best & most consistent way to do it so it works across design-systems.

Agree 👍🏻
One thing I don't understand is this part:

slotProps: {
  component: T
},

Did you mean this instead?

type GridFilterInputComponentProps<T> = {
  item: GridFilterItem;
  // ...
  slotProps: T
}

// e.g.
function TextFieldInput(props: GridFilterInputComponentProps<TextFieldProps>) {
 // ...
}
<TextFieldInput slotProps={{ variant: 'outlined' }} />

Copy link
Member

@cherniavskii cherniavskii Jan 17, 2025

Choose a reason for hiding this comment

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

I'm thinking about renaming it GridFilterInputComponentProps

Sounds good, the current name is somewhat confusing.
A shorter GridFilterInputProps might be good too, even though it doesn't explicitly refer to InputComponent in the name.

Copy link
Contributor Author

@romgrk romgrk Jan 17, 2025

Choose a reason for hiding this comment

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

One thing I don't understand is this part / Did you mean this instead?

Yeah kinda, but slotProps have the convention of always targetting specific slots (e.g. slotProps.input), using the slotProps object as props directly feels like a break in consistency that I'd rather not make, even if it makes things a bit more verbose, so it would end up like this:

// e.g.
<TextFieldInput slotProps={{ textField: { variant: 'outlined' } }} />
// "textField" or "component" maybe, the wrapped component is not necessarily a textfield

Copy link
Member

Choose a reason for hiding this comment

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

Yeah kinda, but slotProps have the convention of always targetting specific slots (e.g. slotProps.input)

Right, now it makes sense.

// "textField" or "component" or whatever

Would root make sense in this case?

Copy link
Contributor Author

@romgrk romgrk Jan 17, 2025

Choose a reason for hiding this comment

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

A shorter GridFilterInputProps might be good too, even though it doesn't explicitly refer to InputComponent in the name.

Yeah that sounds good, our naming & coding guidelines create overly verbose code :/ I'll do that in a separate PR to facilitate the review.

Comment on lines +91 to +98
slotProps?: {
input?: {
disabled?: boolean;
endAdornment?: React.ReactNode;
startAdornment?: React.ReactNode;
};
htmlInput?: React.InputHTMLAttributes<HTMLInputElement>;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved everything to slotProps.{htmlInput,input,inputLabel} instead of {input,Input,InputLabel}Props, because the core has deprecated the latter and will remove it in v7.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 17, 2025
Comment on lines +156 to +164
return (
<MUITextField
variant='outlined'
{...rest}
InputLabelProps={{
shrink: true,
}}
/>
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@KenanYusuf Design-wise, is it ok if all our textfields have shrink: true?

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

Successfully merging this pull request may close these issues.

4 participants