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

[core] Create dedicated InputComponents for single select and date filters #3227

Merged
merged 13 commits into from
Nov 29, 2021

Conversation

alexfauquette
Copy link
Member

Context: #3198 (comment)

I have a doubt about the second commit. If the GridFilterInputValue component is used by developers, removing the support for singleSelect type will be a breaking change

@alexfauquette alexfauquette changed the title [core] create a dedicated InputComponent for single select filter [core] Create a dedicated InputComponent for single select filter Nov 19, 2021
@alexfauquette alexfauquette requested review from DanailH, m4theushw and flaviendelangle and removed request for DanailH November 19, 2021 17:35
@alexfauquette alexfauquette self-assigned this Nov 19, 2021
@m4theushw
Copy link
Member

If the GridFilterInputValue component is used by developers, removing the support for singleSelect type will be a breaking change

We should keep the logic for the singleSelect inside it to not create a breaking change. However, we can add a deprecation message to redirect users to use the new component, only if they use it with a column with type singleSelect.

As example: https://github.com/mui-org/material-ui/blob/857c7ad547207b647094c97be69385c7d921ddc2/packages/mui-material/src/styles/adaptV4Theme.js#L4

@alexfauquette
Copy link
Member Author

I have put Date inputs in a dedicated component because I was looking to modify their InputProps #3222

@alexfauquette alexfauquette changed the title [core] Create a dedicated InputComponent for single select filter [core] Create a dedicated InputComponents for single select and date filters Nov 22, 2021
);
};

export interface GridFilterInputSingleSelectProps extends GridFilterInputValueProps {
Copy link
Member

Choose a reason for hiding this comment

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

We don't we merge the props with TextFieldProps here instead of below ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Seems that we can not extend with a union of type, so we cannot do

export interface GridFilterInputSingleSelectProps extends GridFilterInputValueProps, TextFieldProps {
  type?: 'singleSelect';
}

Copy link
Member

Choose a reason for hiding this comment

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

In that case I think we should use a type

export type GridFilterInputSingleSelectProps = GridFilterInputValueProps & TextFieldProps & { type?: 'singleSelect' }

It's not great, but exporting an incomplete interface seems worse to me

Copy link
Member Author

Choose a reason for hiding this comment

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

I corrected it for the two new components. I can not do it for theGridFilterInputValue because the type GridFilterInputValueProps is already exported. I add it as a comment in the breaking change issue

@alexfauquette alexfauquette changed the title [core] Create a dedicated InputComponents for single select and date filters [core] Create dedicated InputComponents for single select and date filters Nov 29, 2021
@alexfauquette alexfauquette merged commit 1a4324c into mui:master Nov 29, 2021
@alexfauquette alexfauquette deleted the split-filterInput-components branch November 29, 2021 10:22
shrink: true,
}}
inputRef={focusElementRef}
{...other}
Copy link
Member

Choose a reason for hiding this comment

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

This is not a problem here, but per convention other should always be last prop to be spread.

@m4theushw m4theushw mentioned this pull request Apr 27, 2022
41 tasks
@zannager zannager added the core Infrastructure work going on behind the scenes label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants