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

Emit onChange with null when DateInput is empty #524

Merged
merged 3 commits into from
Jan 26, 2017

Conversation

leebyp
Copy link
Contributor

@leebyp leebyp commented Jan 20, 2017

Fixes #504, Fixes #546

Changes proposed in this pull request:

  • Return null as the date argument in DateInput's onChange handler

@blueprint-bot
Copy link

emit onchange with null when date is empty

Preview: docs
Coverage: core | datetime

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.

can we get a unit test? 🥁

@blueprint-bot
Copy link

fix linting

Preview: docs
Coverage: core | datetime

@leebyp leebyp force-pushed the bl/return-null-in-dateinput-is-empty branch from f064ada to ff75b3d Compare January 22, 2017 01:14
@blueprint-bot
Copy link

unify interactions

Preview: docs
Coverage: core | datetime

@blueprint-bot
Copy link

fix linting

Preview: docs
Coverage: core | datetime

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.

would've liked the tests refactor as a separate PR but oh well, looks good!

@adidahiya
Copy link
Contributor

can we have it return undefined instead of null? I would really like to avoid introducing null at the API boundaries that we control.

@giladgray
Copy link
Contributor

@adidahiya i was wondering the same thing! ☁️

DateRange uses null to mean "no date", though its typings declare Date | undefined. someone is definitely wrong. we should fix this whole thing at once, not in this PR.

i think it should wait for strictNullChecks #325 because that'll make it so much easier to test.

@giladgray giladgray merged commit 0103132 into master Jan 26, 2017
@giladgray giladgray deleted the bl/return-null-in-dateinput-is-empty branch January 26, 2017 18:50
@leebyp
Copy link
Contributor Author

leebyp commented Jan 26, 2017

i mainly followed the convention from DatePicker but yup, all three should be doing the same thing whichever we decide. I prefer null because undefined is already tied to the concept on "uncontrolled components" in the BP world.

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.

4 participants