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

Fixed number input label issue #6361

Conversation

VikrantShirvankar
Copy link
Contributor

@VikrantShirvankar VikrantShirvankar commented Jun 15, 2021

Closes #6352

Fixed label issue for Number input.

Before - with label={false}

Screenshot 2021-06-15 at 11 27 48 PM

/>
label !== '' &&
label !== false && (
<FieldTitle
Copy link
Contributor

Choose a reason for hiding this comment

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

It would probably be better to handle this in FieldTitle directly. Then we would only have to update the propTypes in all the other components

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Handled label issue directly inside FieldTitle. Updated propType for NumberInput field.
I will raise separate PR to update label propType in other components. Will it be fine?
Let me know if any changes required.
Thanks..!

Copy link
Member

Choose a reason for hiding this comment

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

Please update the propTypes and TypeScript types in this PR

@djhi
Copy link
Contributor

djhi commented Jun 16, 2021

Thanks!

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Thanks for your PR.

You should also refactor TextInput, SelectInput and RichTextInput to remove the test on label !== false, which is now redundant.

You should document that feature in the Input documentation.

Finally, this is technically a new feature, and as such it should be PRed against next rather than master.

@@ -18,9 +18,15 @@ export const FieldTitle: FunctionComponent<Props> = ({
isRequired,
}) => {
const translate = useTranslate();

if (label === false && label === '') {
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't make sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. So the desired behavior is to return null only if label is false. Right? Correct me if I am wrong.

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 just followed below existing code -
https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/input/TextInput.tsx#L65
As per this code, FieldTitle will not get rendered if string is empty.

Copy link
Member

Choose a reason for hiding this comment

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

my bad, you're right. Both '' and false should disable the title.

expect(container.firstChild).toBeNull();
});

it('should return null if label is empty string', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the desired behavior. An empty string should lead to an empty label (not the absence of label)

@VikrantShirvankar
Copy link
Contributor Author

@fzaninotto fzaninotto closed this Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Many of inputs don't support label={false}
3 participants