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.

Sign up for GitHub

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

Jump to bottom

Fix <TextInput> missing placeholder with MUI v5.4 #8439

Merged
merged 7 commits into from
Dec 8, 2022
Merged

Fix <TextInput> missing placeholder with MUI v5.4 #8439

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cbb47c6
Pass `undefined` as `label` to MUI if `false` or an empty string is p…
kosmotema Nov 26, 2022
ae9418b
Prevent overriding `label` prop in SearchInput
kosmotema Nov 26, 2022
9b7670a
Add back `hiddenLabel` for SearchInput
kosmotema Nov 28, 2022
41c54b0
Add more tests to cover TextInput's label prop
kosmotema Nov 28, 2022
11036ef
Use `null` instead of `undefined` when label should not be displayed
kosmotema Nov 28, 2022
a30786c
Merge branch 'master' into input-label
kosmotema Nov 28, 2022
3e0554e
Merge branch 'master' into input-label
kosmotema Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions packages/ra-ui-materialui/src/input/SearchInput.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import * as React from 'react';
import { render } from '@testing-library/react';
import { testDataProvider } from 'ra-core';

import { AdminContext } from '../AdminContext';
import { SearchInput } from '.';
import { FilterForm } from '../list';

describe('<SearchInput />', () => {
it('should not render label if passed explicit `undefined` value', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added to cover the case of passing explicit undefined for the SearchInput's label property (more info here)

const source = 'test';

const filters = [<SearchInput source={source} label={undefined} />];
const displayedFilters = {
[source]: true,
};

const { container } = render(
<AdminContext dataProvider={testDataProvider()}>
<FilterForm
setFilters={jest.fn()}
filters={filters}
displayedFilters={displayedFilters}
/>
</AdminContext>
);

expect(container.querySelector(`label`)).toBeNull();
});
});
6 changes: 4 additions & 2 deletions packages/ra-ui-materialui/src/input/SearchInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,11 @@ import { CommonInputProps } from './CommonInputProps';
import { TextInput, TextInputProps } from './TextInput';

export const SearchInput = (props: SearchInputProps) => {
const { label, ...rest } = props;
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you made this change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To handle the case when specifying undefined for a SearchInput's label explicitly (like <SearchInput source="q" alwaysOn label={undefined} />) will result in the default label is being displayed.
Reproduction on codesandbox

Copy link
Contributor Author

@kosmotema kosmotema Nov 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also noticed, that it's possible to use SearchInput with 0 label like (added this case to repro on CodeSandbox):

<SearchInput source="q" alwaysOn label={0} />

I think, it would be better to revert all changes in this PR, that related to the SearchInput's label overwriting logic and open a new issue to discuss, what and how to do about this case (IMHO it's better to prevent users from providing a label at all instead of throwing an error).


const translate = useTranslate();

if (props.label) {
if (label) {
throw new Error(
"<SearchInput> isn't designed to be used with a label prop. Use <TextInput> if you need a label."
);
Expand All @@ -30,7 +32,7 @@ export const SearchInput = (props: SearchInputProps) => {
),
}}
size="small"
{...props}
{...rest}
/>
);
};
Expand Down
62 changes: 62 additions & 0 deletions packages/ra-ui-materialui/src/input/TextInput.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,66 @@ describe('<TextInput />', () => {
expect(input.value).toEqual('bar');
});
});

describe('label', () => {
it('should render label when `label` prop not specified', () => {
const { container } = render(
<AdminContext dataProvider={testDataProvider()}>
<SimpleForm
defaultValues={{ title: 'hello' }}
onSubmit={jest.fn}
>
<TextInput {...defaultProps} />
</SimpleForm>
</AdminContext>
);

expect(container.querySelector(`label`)).not.toBeNull();
});

it('should render label when `label` prop is non-empty string', () => {
const { container } = render(
<AdminContext dataProvider={testDataProvider()}>
<SimpleForm
defaultValues={{ title: 'hello' }}
onSubmit={jest.fn}
>
<TextInput {...defaultProps} label="label" />
</SimpleForm>
</AdminContext>
);

expect(container.querySelector(`label`)).not.toBeNull();
});

it('should not render label when `label` prop is `false`', () => {
Copy link
Member

Choose a reason for hiding this comment

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

You're missing a test showing that a proper label prop renders a label.

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've added two more tests: when label not specified and when label is a non-empty string

const { container } = render(
<AdminContext dataProvider={testDataProvider()}>
<SimpleForm
defaultValues={{ title: 'hello' }}
onSubmit={jest.fn}
>
<TextInput {...defaultProps} label={false} />
</SimpleForm>
</AdminContext>
);

expect(container.querySelector(`label`)).toBeNull();
});

it('should not render label when `label` prop is empty string', () => {
const { container } = render(
<AdminContext dataProvider={testDataProvider()}>
<SimpleForm
defaultValues={{ title: 'hello' }}
onSubmit={jest.fn}
>
<TextInput {...defaultProps} label="" />
</SimpleForm>
</AdminContext>
);

expect(container.querySelector(`label`)).toBeNull();
});
});
});
5 changes: 2 additions & 3 deletions packages/ra-ui-materialui/src/input/TextInput.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,15 +64,14 @@ export const TextInput = (props: TextInputProps) => {
{...field}
className={clsx('ra-input', `ra-input-${source}`, className)}
label={
label !== '' &&
label !== false && (
label !== '' && label !== false ? (
<FieldTitle
label={label}
source={source}
resource={resource}
isRequired={isRequired}
/>
)
) : null
}
error={(isTouched || isSubmitted) && invalid}
helperText={
Expand Down