-
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(TimePicker): reset input fields when value={null} #4845
[datetime] fix(TimePicker): reset input fields when value={null} #4845
Conversation
Thanks for your interest in palantir/blueprint, @dummycode! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
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, simple fix. I'll make sure to note this change in behavior in the release notes.
packages/docs-app/src/examples/datetime-examples/dateTimePickerExample.tsx
Outdated
Show resolved
Hide resolved
@@ -100,7 +100,11 @@ export class DateTimePicker extends AbstractPureComponent2<IDateTimePickerProps, | |||
onChange={this.handleDateChange} | |||
value={value} | |||
/> | |||
<TimePicker {...this.props.timePickerProps} onChange={this.handleTimeChange} value={value} /> | |||
<TimePicker |
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.
this component is deprecated, please revert these changes. users should be using <DatePicker>
instead.
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.
Without this change, the test changing the time before selecting a date works as expected
fails because clearing the date also clears the time because the TimePicker
component now responds to being passed null
, whereas before it was a no-op. Should I still revert this change?
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, fair enough, we can include this change in the PR.
@@ -222,9 +215,12 @@ export class TimePicker extends React.Component<TimePickerProps, ITimePickerStat | |||
const didMaxTimeChange = prevProps.maxTime !== this.props.maxTime; | |||
const didBoundsChange = didMinTimeChange || didMaxTimeChange; | |||
const didPropValueChange = prevProps.value !== this.props.value; | |||
const shouldStateUpdate = didMinTimeChange || didMaxTimeChange || didBoundsChange || didPropValueChange; |
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.
good catch... this expression was redundant
This reverts commit 56b374c.
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.
looks good in docs preview
@@ -100,7 +100,11 @@ export class DateTimePicker extends AbstractPureComponent2<IDateTimePickerProps, | |||
onChange={this.handleDateChange} | |||
value={value} | |||
/> | |||
<TimePicker {...this.props.timePickerProps} onChange={this.handleTimeChange} value={value} /> | |||
<TimePicker |
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, fair enough, we can include this change in the PR.
Fixes #4775
Checklist
Changes proposed in this pull request:
Updating
TimePickers
value to null should behave the same way as when it was constructed: get min time or default value.Reviewers should focus on:
Whether this breaks any existing functionality.
Screenshot