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

[TypeScript] Fix FilterLiveSearchProps should extend TextInputProps #7859

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

antoinefricker
Copy link
Contributor

Fixes #7832

@antoinefricker antoinefricker changed the title [TS] FilterLiveSearchProps should extend TextInputProps [TypeScript] Fix FilterLiveSearchProps should extend TextInputProps Jun 20, 2022
@antoinefricker antoinefricker added the RFR Ready For Review label Jun 20, 2022
@vercel vercel bot temporarily deployed to Preview – react-admin June 20, 2022 09:15 Inactive
source?: string;
sx?: SxProps;
label?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should override the label prop as we forward it as is to the TextInput. We only provide a default

Copy link
Contributor

Choose a reason for hiding this comment

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

I would tend to agree with @septentrion-730n because his solution still provides a default value and also supports being overridden by the user directly in the FilterLiveSearch component.
Previously the only way you could customize the label was through localization.

However, it could be nice to handle translation on this field either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

The TextInput already support all these use cases: https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/input/TextInput.tsx#L66 via FieldTitle.

Btw, we don't event need to translate here, we could just pass the translation key directly as label and it will be translated

const {
source = 'q',
variant,
label = translate('ra.action.search'),
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't even need to translate here, just pass the translation key directly to the label prop. <TextInput> will handle the translation

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, accepting react node for this label would complicate the code as we only use it as the TextInput label when the variant is outlined. Otherwise, we pass it as the placeholder prop

const {
source = 'q',
variant,
label = translate('ra.action.search'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, accepting react node for this label would complicate the code as we only use it as the TextInput label when the variant is outlined. Otherwise, we pass it as the placeholder prop

@djhi djhi added this to the 4.1.6 milestone Jun 20, 2022
@djhi djhi merged commit 5c1122a into master Jun 20, 2022
@djhi djhi deleted the 7832-filterLiveSearch-props branch June 20, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FilterLiveSearchProps should extend TextInputProps
3 participants