Skip to content

Fix end time selection before selecting date #6858

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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Jul 2, 2024
Merged

Fix end time selection before selecting date #6858

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d8d9fc6
Fix end time selection before selecting date
evansjohnson Jun 24, 2024
1ce57b1
add tests
evansjohnson Jun 24, 2024
021d4a2
lint
evansjohnson Jun 24, 2024
5e01be0
prettier and test fix
evansjohnson Jun 24, 2024
603e0cd
fix test
evansjohnson Jun 25, 2024
538464f
Update packages/datetime2/src/components/date-range-picker3/dateRange…
evansjohnson Jun 28, 2024
49f8344
Update packages/datetime2/src/components/date-range-picker3/dateRange…
evansjohnson Jun 28, 2024
6e133a2
Add generated changelog entries
svc-changelog Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update packages/datetime2/src/components/date-range-picker3/dateRange… 
…Picker3.tsx
  • Loading branch information
@evansjohnson
evansjohnson authored Jun 28, 2024
commit 538464f2626b5a74bf0c44e4f155a436d89b29ad
4 changes: 4 additions & 0 deletions packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ export class DateRangePicker3 extends DateFnsLocalizedComponent<DateRangePicker3
this.setState({ value: newDateRange, time: newTimeRange });
};

// When a user sets the time value before choosing a date, we need to pick a date for
// them to have something to store the time on.
// The default depends on the value of the other date since there's an invariant
// that the left/0 date is always less than the right/1 date.
private getDefaultDate = (dateIndex: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. When someone sets the time value before choosing a date, we need to "infer" or pick a date for them to initialize.
  2. 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.

const { value } = this.state;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree! Let's leave as-is. I almost didn't make this comment because it was an existing issue.

const otherIndex = dateIndex === 0 ? 1 : 0;
Expand Down