-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
There was a problem hiding this 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'; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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:
- return the emptyText without formatting by default (letting the developer pass an element wrapped inside Typography to let them style the empty text)
- 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?
Ok, I'll standardize all components to return a Typography with |
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. |
Thanks! |
Closes #2544
TODO:
- Fix docs- Merge againstnext