-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
[datetime] fix(DatePicker, TimePicker): allow value prop to be null #4274
[datetime] fix(DatePicker, TimePicker): allow value prop to be null #4274
Conversation
packages/datetime/src/dateInput.tsx
Outdated
@@ -84,7 +84,7 @@ export interface IDateInputProps extends IDatePickerBaseProps, IDateFormatProps, | |||
/** | |||
* The default date to be used in the component when uncontrolled. | |||
*/ | |||
defaultValue?: Date; | |||
defaultValue?: Date | null; |
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 would this ever be null
?
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.
When working with form libraries like react-hook-form, using the Controller requires setting a defaultValue even if no initial value is intended, if you pass undefined it acts as you didn't set the parameter, so only "null" is the shared value that both Controller and DateInput uses for "empty state".
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 unfortunate, I wish we didn't have to add extra surface to the API just to support a library like react-hook-form. I think I understand the problem, but just to be clear, can you create a code sandbox clearly demonstrating the issue?
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.
@adidahiya I understand, however typings are for being used, and if value is already (Date | null | undefined) I don't see why defaultValue would be treated differently.
Here's the sanbox illustrating the issue
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.
Ok, thanks for that sandbox, it's helpful. In my understanding of controlled form components, I treat defaultValue
and value
differently. value
is allowed to be null
because that means you're controlling it with the intention of changing value
later, but you don't have a valid value to set yet. This is different from value={undefined}
, which means you are leaving the component uncontrolled. defaultValue
, on the other hand, does not need the distinction between undefined
and null
type values. Both effectively mean the same thing. So my thinking goes: why leave ambiguity for the developer using Blueprint? Just pick the value which is closer to the semantic meaning (defaultValue={undefined}
means "I don't have any default to set, just let the component decide").
Why does this issue not come up with @types/react
definitions for <input defaultValue>
(defined here)? Those types don't include null
.
All that being said, I'd generally be ok with this change going through. The logic around defaultValue
is set up to deal with null
correctly, so it's really just a matter of tweaking the public API types.
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.
I've checked again the source for rhf and defaultValue actually refers to the component value, not the html prop defaultValue, so it seems making a reusable component with them requires defining a custom defaultValue picking the type of DateInput value. In that case and also checking @types/react defaults, there's no more need to alter defaultValues types, so I will remove them from this PR.
packages/datetime/src/datePicker.tsx
Outdated
@@ -56,7 +56,7 @@ export interface IDatePickerProps extends IDatePickerBaseProps, IProps { | |||
* Initial day the calendar will display as selected. | |||
* This should not be set if `value` is set. | |||
*/ | |||
defaultValue?: Date; | |||
defaultValue?: Date | null; |
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.
same question here
@@ -31,7 +31,7 @@ export interface IDateTimePickerProps extends IProps { | |||
* This will be ignored if `value` is set. | |||
* @default Date.now() | |||
*/ | |||
defaultValue?: Date; | |||
defaultValue?: Date | null; |
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.
same question here
packages/datetime/src/timePicker.tsx
Outdated
@@ -59,7 +59,7 @@ export interface ITimePickerProps extends IProps { | |||
* Initial time the `TimePicker` will display. | |||
* This should not be set if `value` is set. | |||
*/ | |||
defaultValue?: Date; | |||
defaultValue?: Date | null; |
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.
same question here
Changes proposed in this pull request:
Adds null typings to datetime interfaces to match clear/empty value