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

[DateTime] Add isUserChange support for DateInput #2573

Merged
merged 5 commits into from
Jun 11, 2018

Conversation

qcz
Copy link
Contributor

@qcz qcz commented Jun 6, 2018

Fixes #2418

Checklist

Changes proposed in this pull request:

  • Add isUserChange support for DateInput to be able to distinguish month/year changes from real date selections.
  • Change the name of the second parameter of the onChange handler in IDatePickerProps to isUserChange, so all supported components use the same argument name.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

this looks great! just a quick question.

@@ -73,9 +73,10 @@ export interface IDateInputProps extends IDatePickerBaseProps, IDateFormatProps,

/**
* Called when the user selects a new valid date through the `DatePicker` or by typing
* in the input.
* in the input. The second argument is true if and only if the user clicked on a date in the
* calendar; it will be false if the date was changed by choosing a new month or year.
Copy link
Contributor

Choose a reason for hiding this comment

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

@qcz i just edited these docs a bit. question: is the arg true if the user types in a new date in the input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! The current solution treats any real user input (selecting a date in the datepicker/datetimepicker, changing the input, clearing the date) as a manual change event, so only change events fired by year and month change is non-manual. As I usually set readonly in the inputProps to readonly, so users can only change the date by selecting a date in the datepicker, I did not really thought of the naming convention.

I think that isUserChange should be a better name choice (and it is already used by DateTimePickers). Maybe we should change the name of the argument to the same for DatePickers too so it will be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to isUserChange, please do that!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@giladgray: done!

@qcz
Copy link
Contributor Author

qcz commented Jun 6, 2018

update prop docs

Preview: documentation | landing | table

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

isUserChange please

@qcz qcz changed the title [DateTime] Add hasUserManuallySelectedDate support for DateInput [DateTime] Add isUserChange support for DateInput Jun 8, 2018
@qcz
Copy link
Contributor Author

qcz commented Jun 8, 2018

hasUserManuallySelectedDate → isUserChange

Preview: documentation | landing | table

@qcz
Copy link
Contributor Author

qcz commented Jun 8, 2018

Fix lint error

Preview: documentation | landing | table

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

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

👏 👏 very nice work @qcz! thank you!

this will ship in the next 3.0 beta release later this week.

@giladgray giladgray merged commit 81a69eb into palantir:develop Jun 11, 2018
@qcz
Copy link
Contributor Author

qcz commented Jun 12, 2018

@giladgray: Thanks! Looking forward to it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants