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

[RFR] Add emptyText prop to show a fixed string when the value of a field is null #4413

Merged
merged 14 commits into from
Feb 20, 2020

Conversation

m4theushw
Copy link
Contributor

@m4theushw m4theushw commented Feb 12, 2020

Closes #2544

TODO:
- Fix docs
- Merge against next

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.

Nice!

Don't forget to update the documentation about common field props, too.

@@ -1,7 +1,7 @@
import React from 'react';
import assert from 'assert';
import { shallow } from 'enzyme';
import EmailField from './EmailField';
import { EmailField } from './EmailField';
Copy link
Member

Choose a reason for hiding this comment

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

why did you change that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The recompose/pure in EmailField was not playing well with enzyme.

@m4theushw m4theushw changed the base branch from master to next February 16, 2020 03:48
@m4theushw m4theushw changed the title [WIP] Add emptyText prop to show a fixed string when the value of a field is null Add emptyText prop to show a fixed string when the value of a field is null Feb 17, 2020
@m4theushw m4theushw requested a review from fzaninotto February 17, 2020 02:33
@m4theushw m4theushw changed the title Add emptyText prop to show a fixed string when the value of a field is null [RFR] Add emptyText prop to show a fixed string when the value of a field is null Feb 17, 2020
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.

That's great!

I have a general concern about how the emptyText renders in your PR. In some components, you put it inside another element (e.g. ChipField), in others, inside a span with the className, and in some others, you just render the emptyText directly.

I think all the components should behave the same. There are 2 options:

  1. return the emptyText without formatting by default (letting the developer pass an element wrapped inside Typography to let them style the empty text)
  2. return the emptyText formatted just like a regular content

I think 2. is preferable (because of the principle of least WTF). What's your opinion?

@m4theushw
Copy link
Contributor Author

Ok, I'll standardize all components to return a Typography with emptyText inside. Maybe in the future depending on the acceptance by the public we can allow to override by passing an element.

@fzaninotto
Copy link
Member

If the developer wants a custom component rendered in case of null value, they can still wrap the Field inside a component of their own.

@m4theushw m4theushw requested a review from fzaninotto February 18, 2020 03:33
@fzaninotto fzaninotto merged commit acad6d7 into marmelab:next Feb 20, 2020
@fzaninotto
Copy link
Member

Thanks!

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.

Allow textual empty label as a property in fields and inputs
2 participants