Skip to content
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

Fix end time selection before selecting date #6858

Merged
merged 8 commits into from
Jul 2, 2024

Conversation

evansjohnson
Copy link
Contributor

@evansjohnson evansjohnson commented Jun 24, 2024

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

  • Includes tests
  • [-] Update documentation

Changes proposed in this pull request:

  • Fix behavior of default date such that selected range cannot end up reversed.
  • When allowSingleDayRange is not true, 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:

  • There's a slight behavior break in that users could previously select a single day range by modifying the time with today's date selected as the start date. This seems like a bug that's okay to fix but would like a second opinion on this.
  • Any better solutions than to add/subtract a day from the default when allowSingleDayRange is not true?
  • Would you prefer to see the allowSingleDayRange change split out?

@changelog-app
Copy link

changelog-app bot commented Jun 24, 2024

Generate changelog in packages/datetime2/changelog/@unreleased

What do the change types mean?
  • feature: A new feature of the service.
  • improvement: An incremental improvement in the functionality or operation of the service.
  • fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
  • break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
    and external consumers of the service's API (e.g. customer-written software or integrations).
  • deprecation: Advertises the intention to remove service functionality without any change to the
    operation of the service itself.
  • manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
    performing database surgery, ...) at the time of upgrade for it to succeed.
  • migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?
  • ❗The break and manual task changelog types will result in a major release!
  • 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
  • ✨ All others will result in a minor version release.

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Fix DateRangePicker end time selection before selecting end date

Check the box to generate changelog(s)

  • Generate changelog entry

@svc-palantir-github
Copy link

Fix end time selection before selecting date

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@svc-palantir-github
Copy link

lint

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@svc-palantir-github
Copy link

prettier and test fix

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@svc-palantir-github
Copy link

fix test

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@evansjohnson evansjohnson requested a review from invliD June 26, 2024 20:38
@gluxon gluxon self-assigned this Jun 28, 2024
gluxon
gluxon previously approved these changes Jun 28, 2024
Copy link
Contributor

@gluxon gluxon left a comment

Choose a reason for hiding this comment

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

Thank you!

this.props.onChange?.(newDateRange);
this.setState({ value: newDateRange, time: newTimeRange });
};

private getDefaultDate = (dateIndex: number) => {
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.

this.props.onChange?.(newDateRange);
this.setState({ value: newDateRange, time: newTimeRange });
};

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.

@policy-bot policy-bot bot dismissed gluxon’s stale review June 28, 2024 18:47

Invalidated by push of 538464f

@svc-palantir-github
Copy link

Update packages/datetime2/src/components/date-range-picker3/dateRangePicker3.tsx

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

gluxon
gluxon previously approved these changes Jul 1, 2024
Copy link
Contributor

@gluxon gluxon left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks for the comment on getDefaultDate.

@policy-bot policy-bot bot dismissed gluxon’s stale review July 1, 2024 18:51

Invalidated by push of 6e133a2

@svc-palantir-github
Copy link

Add generated changelog entries

Build artifact links for this commit: documentation | landing | table | demo

This is an automated comment from the deploy-preview CircleCI job.

@evansjohnson evansjohnson merged commit f2e9d6e into develop Jul 2, 2024
13 checks passed
@evansjohnson evansjohnson deleted the evanj/daterange-end-time-fix branch July 2, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants