Fix end time selection before selecting date #6858
Conversation
Generate changelog in
|
Fix end time selection before selecting dateBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
lintBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
prettier and test fixBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
fix testBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
| this.props.onChange?.(newDateRange); | ||
| this.setState({ value: newDateRange, time: newTimeRange }); | ||
| }; | ||
|
|
||
| private getDefaultDate = (dateIndex: number) => { | ||
| const { value } = this.state; |
There was a problem hiding this comment.
nitpick: I'm not too worried about this since this is already happening in the existing code, but I wonder if we need to grab existing state in the setState callback rather than reading directly off of this.state.
Otherwise we could end up with weird bugs if this.state is referring to stale values.
In general, any time "next state" is computed from some kind of prior state, I usually read the prior state in a setState callback.
There was a problem hiding this comment.
oof yea, I remember dealing with stuff like this back when seeing class components more often
in this case I'm thinking let's leave as is since as you call out we access this.state just before calling this helper method
| this.props.onChange?.(newDateRange); | ||
| this.setState({ value: newDateRange, time: newTimeRange }); | ||
| }; | ||
|
|
||
| private getDefaultDate = (dateIndex: number) => { |
There was a problem hiding this comment.
Just as a suggestion, I think we can add an inline source code comment to explain why we need to be careful about selecting the next default date.
Maybe something around:
- When someone sets the time value before choosing a date, we need to "infer" or pick a date for them to initialize.
- The next default date for the 0 or 1 index depends on the value of the other index since there's an invariant that the left/0 date is always less than the right/1 date.
packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsx
Show resolved
Hide resolved
packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsx
Outdated
Show resolved
Hide resolved
Update packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsxBuild artifact links for this commit: documentation | landing | table | demoThis is an automated comment from the deploy-preview CircleCI job. |
Fixes #6863
Fixes #6856
Previously, users could end up with a reversed selection range if modifying the time input of the end date before selecting a date. Today's date was used as the default in these cases, leaving the user with a reversed range if the start date was later than today's date.
Checklist
Changes proposed in this pull request:
allowSingleDayRangeis nottrue, ensure the default date is 1 day off from the already selected date (otherwise keep today's date as default if no dates are selected).Reviewers should focus on:
allowSingleDayRangeis nottrue?allowSingleDayRangechange split out?