[fields] Allow empty textField slot placeholder value
#13148
Conversation
arthurbalduini
added
bug 🐛
|
Deploy preview: https://deploy-preview-13148--material-ui-x.netlify.app/ |
arthurbalduini
changed the title
arthurbalduini
changed the title
| @@ -390,8 +390,8 @@ export const useFieldV6TextField: UseFieldTextField<false> = (params) => { | |||
| }); | |||
|
|
|||
| const placeholder = React.useMemo(() => { | |||
| if (inPlaceholder) { | |||
| return inPlaceholder; | |||
| if (params.forwardedProps.hasOwnProperty('placeholder')) { | |||
There was a problem hiding this comment.
How about simply doing:
| if (params.forwardedProps.hasOwnProperty('placeholder')) { | |
| if (inPlaceholder !== undefined) { |
I think that it would be more consistent with the other props in our codebase.
Usually when you pass undefined, we fallback to the internal value rather than applying undefined (for example in props.value or props.views)
There was a problem hiding this comment.
My thought to decide to use this approach was to be able to differentiate the scenarios:
- user explicitly passes the placeholder value as undefined
- user doesn't pass any placeholder prop
In both cases inPlaceholder will be undefined, but on the second scenario, we don't pass forward the prop, so the params.forwardedProps.hasOwnProperty('placeholder') will be false and it will fall on the default placeholder, on the other hand if users explicitly passes the prop as undefined, I assume they wish to have an empty placeholder. WDYT ?
There was a problem hiding this comment.
I totally understand your approach
IMHO, we should render the default placeholder when the user renders <DateField placeholder={undefined} />, because it's closer from the other props / components behavior (<DateField minDate={undefined} /> will apply the 01-01-1900 as the min date rather than removing the boundary, <DateField value={undefined} /> will fallback to the internal state rather than enforcing an empty value, etc...).
There was a problem hiding this comment.
I agree with @flaviendelangle on this one, let's go for simplicity. 👍
In "my book", undefined == no prop.
There was a problem hiding this comment.
Thanks, folks. Done in 658c29c
| @@ -400,6 +400,7 @@ export const useFieldV6TextField: UseFieldTextField<false> = (params) => { | |||
| isRTL, | |||
| ); | |||
| }, [ | |||
| params.forwardedProps, | |||
There was a problem hiding this comment.
This could force the placeholder to be recalculated more often than we'd want. 🙈
| if (inPlaceholder) { | ||
| return inPlaceholder; | ||
| if (params.forwardedProps.hasOwnProperty('placeholder')) { | ||
| return inPlaceholder ?? ''; |
There was a problem hiding this comment.
Have you tried testing Flavien's suggestion?
I think that with it we could remove the ?? '' here. 🤔
There was a problem hiding this comment.
And remove params.forwardedProps from the deps
LukasTy
changed the title
LukasTy
changed the title
textFieldslot placeholder value
LukasTy
changed the title
textFieldslot placeholder valuetextField slot placeholder value
packages/x-date-pickers/src/DesktopDatePicker/tests/field.DesktopDatePicker.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Lukas <[email protected]> Signed-off-by: Arthur Suh Balduini <[email protected]>
packages/x-date-pickers/src/DesktopDatePicker/tests/field.DesktopDatePicker.test.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Flavien DELANGLE <[email protected]> Signed-off-by: Arthur Suh Balduini <[email protected]>
Fixes #12573.
Add some tests to prevent it in the future.
It should now be possible to pass the
placeholderprop as either an empty string or undefined in order to achieve an empty placeholder in textField component. Sending no placeholder prop must fall into the default placeholder, generated specifically for each kind of picker.