[DateTimePicker] Fix where changing TimePicker clears date

[cathyxz]

Fixes #1402

Checklist

• Enable CircleCI for your fork
• Add unit tests

Changes proposed in this pull request:

In dateTimePicker component, we should only reset the state based on props if the props have actually changed. Otherwise, we will needless clear or reset DateTimePicker state based on props that have not changed.

Reviewers should focus on:

Screenshot

---

This change is 

[palantirtech]

Thanks for your interest in palantir/blueprint, @cathyxz! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

[giladgray]

Thanks @cathyxz! Would you be able to add a unit test that asserts the bug is fixed? Like, changing time props should not reset date.

[cmslewis]

Thanks! Left a small request.

[cmslewis]

nit: would prefer this structure to avoid nesting everything so much:

if (this.props.value === nextProps.value) {
    return;
} else if (nextProps.value != null) {
    // ....
} else {
    // ...
}

[cathyxz]

I actually like it better too...

[giladgray]

@cathyxz case in point for why you should not force-push: try to click the filename for chris' comment above and you'll see a sad github page. https://github.com/palantir/blueprint/pull/1474/files/4f0d54473b9647c02915d9270b66403fb06f8256

[giladgray]

also @cathyxz can we get that unit test?

[cathyxz]

I added a unit test, but I'm not too sure now about this being the correct fix.

What I did with DateTimePicker was essentially to say that if you pass in the same prop twice, that should not result in a change of state.

Since @cmslewis wrote logic to ensure that passing a null or undefined value clears the selected date, I'm not actually sure whether we should fix the DateTimePicker or the usage of it in our DateTimePickerExample. DateTimePickerExample is kind of weird in the sense that it does not pass a value prop, thus value here defaults to undefined. Usually if you are controlling the DateTimePicker, I imagine you would want to control the value. For example, an equally effective fix for the original problem would be to pass in state.date from the DateTimePickerExample as value. This would entail something more along the lines of a docs clarification. Basically to require that if you pass in an onChange, then you also have to manage the value?

[giladgray]

this behavior seems correct to me? the if / else if / else pattern helps a lot. we must continue to support uncontrolled usage, which necessarily entails supporting onChange without value to simply listen to interactions. this is a standard React pattern.

[giladgray]

would be much clearer to use an actual value and pass it on line 105 too.
<DateTimePicker value={VALUE} />

[cathyxz]

But that does not test the same thing. I think it is more appropriate to change the name of this test to reflect what it's really testing (which I did). So the DateTimePicker only selects its own date and time if the value prop is undefined. What we are trying to fix here is given that a user implements onChange but does NOT pass in a value, we want to avoid the onChange handler, which will pass in new props, to interfere with / clear out the date/time state managed by the DateTimePicker.

For example, this test should fail:

 it("Passing the same value prop twice does not change a user-selected date", () => {
            const value = new Date(2012, 2, 5, 6, 5, 40);
            const { root, getDay } = wrap(<DateTimePicker value={value} />);
            assert.strictEqual(root.state("dateValue"), value);
            
            getDay(15).simulate("click");

            const modifiedDate = new Date(2012, 2, 15, 6, 5, 40);
            assert.strictEqual(root.state("dateValue"), modifiedDate); // this will fail because DateTimePicker no longer manages state since value is defined
            
            root.setProps({ value: value });
            assert.strictEqual(root.state("dateValue"), modifiedDate);
 });

[giladgray]

😄 if you're worried about incorrect behavior, write more unit tests! 🏫

[cmslewis]

Let's keep this newline in place to differentiate external dependencies from internal dependencies.

[cmslewis]

Delete this trailing newline

[giladgray]

👍 newline is legit now that there's a second test

[cmslewis]

Delete this trailing newline

[cathyxz]

@cmslewis I think we're gtg here, but obviously welcome any more feedback and comments.

[cmslewis]

I'll try to add this to the release.