Datepicker: Allow null value for currentDate on mounting #12963

psealock · 2018-12-18T01:34:38Z

Description

When a null value is passed to <DatePicker >, don't set the date to current date. Instead, allow the value to remain null such that no selection is visible on the calendar.

undefined values for currentDate behave as they did previously, which is to fall back to the today's date.

Testing

Confirm that a dependency on setting currentDate to todays date by default does not exist by pulling down the branch and seeing that nothing breaks.
Force a null value for the currentDate prop on <DatePicker > to ensure nothing breaks and no date is selected.
Remove the currentDate prop and see the date picker default to today's date.

gutenberg/packages/components/src/date-time/index.js

Lines 48 to 51 in 0c734e1

    
           <DatePicker 
        
           	currentDate={ currentDate } 
        
           	onChange={ onChange } 
        
           />

Screenshots

Types of changes

New feature: Allow null value of date selection to persist such that no date is selected on the calendar.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

aduth · 2019-01-24T20:18:03Z

Wouldn't this be considered a breaking change for someone who might have relied on the current behavior?

Is a middle-ground option that we allow for an explicit null value to signify "unselected", allowing undefined to continue to represent the current-date defaulting?

psealock · 2019-01-24T22:23:04Z

Great feedback here, thank you @aduth !

I've added a method getMomentDate to explicitly check for null such that undefined values for currentDate have the same experience as before.

aduth · 2019-01-25T19:28:06Z

packages/components/src/date-time/date.js

@@ -25,7 +25,7 @@ class DatePicker extends Component {
 	onChangeMoment( newDate ) {
 		const { currentDate, onChange } = this.props;

-		const momentDate = currentDate ? moment( currentDate ) : moment();
+		const momentDate = this.getMomentDate( currentDate );
 		const momentTime = {
 			hours: momentDate.hours(),


getMomentDate can return null, at which point this would throw an error.

It would be good to document any function we introduce, as it may make this sort of thing easier to catch, when recognizing that the return value of the function is nullable.

Unit tests, as well, would be good to capture this sort of issue and the expected behavior.

Eesh, good call. I've gone back to the original implementation here and included a comment about to explain why moment() is used if currentDate is undefined or null.

Unit tests added. Thanks

packages/components/src/date-time/date.js

aduth · 2019-01-28T19:23:13Z

packages/components/src/date-time/test/date.js

+ */
+import DatePicker from '../date';
+
+describe( 'DatePicker', () => {


I'm glad to see tests, but for what it's worth these tests as written would not have caught the error described in my previous review, since there is zero coverage for onChangeMoment.

psealock · 2019-01-30T00:50:01Z

Thanks for the edits to the jsDoc @aduth.

I added testing to onChangeMoment also.

aduth · 2019-01-30T20:11:36Z

packages/components/src/date-time/test/date.js

+			const onChangeSpy = jest.fn();
+			const wrapper = shallow( <DatePicker onChange={ onChangeSpy } /> );
+			const newDate = moment( '1986-10-18T11:00:00' );
+			const current = moment();


This worries me a bit; I feel like it will be prone to test fragility, where the time required to execute the test is such that moment() called here will produce a different time than moment() called within the implementation of onChangeMoment. Usually this should take a mere fraction of a second, but I've witnessed in the past where it's enough time to tick into the next second and cause an uenxpected test failure.

I think our options would be:

Find a way to provide or otherwise control the current time as referenced within onChangeMoment

Mock moment to have full control over what it returns†

Provide some flexibility in the assertion (i.e. within a +/- allowable timeframe)

Don't test it

Lets go with Provide some flexibility in the assertion (i.e. within a +/- allowable timeframe) for $1000 please, Alex.

I figured comparison on basis of seconds was enough to overcome differences in execution time, but in an effort to prevent future headaches, a change is a good idea.

I changed the tests to save the argument passed to onChange for later comparison using moment's isSame function. Then I compare with a granularity of minute, which would preclude all but the rarest of circumstances where a pause in execution time ticks into the next second AND minute.

psealock · 2019-02-07T20:37:28Z

Hi @aduth, would you have a moment to give this another look?

I changed the tests to save the argument passed to onChange for later comparison using moment's isSame function. Then I compare with a granularity of minute, which would preclude all but the rarest of circumstances where a pause in execution time ticks into the next second AND minute.

Many thanks!

aduth

When it comes to intermittent test failures, I'd prefer "impossible" over "rare", but I'll take what I can get.

Thanks for your patience 👍

aduth · 2019-02-26T19:10:04Z

When it comes to intermittent test failures, I'd prefer "impossible" over "rare", but I'll take what I can get.

Encountered my first failure today:

psealock · 2019-02-27T08:31:50Z

Yikes, thanks for the heads up. I'll look into this to evaluate other solutions

psealock · 2019-03-05T08:33:40Z

@aduth Here is a PR to fix the intermittent failures, #14230

* Datepicker: Allow null value for currentDate on mounting * flexible assertion, FTW

This was referenced Dec 20, 2018

Add Single Date Picker woocommerce/woocommerce-admin#1069

Merged

Single Date Picker: A11y review woocommerce/woocommerce-admin#1145

Closed

psealock mentioned this pull request Jan 1, 2019

Advanced Filters: Add DatePicker woocommerce/woocommerce-admin#1146

Merged

designsimply added [Priority] High Used to indicate top priority items that need quick attention [Package] Date /packages/date labels Jan 17, 2019

designsimply added this to the 4.9 (Gutenberg) milestone Jan 17, 2019

designsimply requested a review from a team January 17, 2019 21:42

psealock force-pushed the update/datepicker-allow-null-on-mount branch from cb3507b to 6535d09 Compare January 20, 2019 19:49

youknowriad modified the milestones: 4.9 (Gutenberg), 5.0 (Gutenberg) Jan 21, 2019

psealock force-pushed the update/datepicker-allow-null-on-mount branch 2 times, most recently from 96eab9a to 477b060 Compare January 23, 2019 20:04

aduth requested changes Jan 25, 2019

View reviewed changes

psealock force-pushed the update/datepicker-allow-null-on-mount branch from a81e205 to 79ff4a3 Compare January 27, 2019 23:17

aduth reviewed Jan 28, 2019

View reviewed changes

psealock force-pushed the update/datepicker-allow-null-on-mount branch from eb11780 to 08578e8 Compare January 30, 2019 00:50

Datepicker: Allow null value for currentDate on mounting

2e40cf6

psealock force-pushed the update/datepicker-allow-null-on-mount branch from 08578e8 to 2e40cf6 Compare January 30, 2019 01:22

aduth reviewed Jan 30, 2019

View reviewed changes

flexible assertion, FTW

b123824

youknowriad modified the milestones: 5.0 (Gutenberg), 5.1 (Gutenberg) Feb 4, 2019

aduth approved these changes Feb 8, 2019

View reviewed changes

aduth merged commit a59aaf2 into WordPress:master Feb 8, 2019

aduth added [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components labels Feb 8, 2019

aduth removed [Priority] High Used to indicate top priority items that need quick attention [Package] Date /packages/date labels Feb 8, 2019

psealock deleted the update/datepicker-allow-null-on-mount branch February 10, 2019 17:53

This was referenced Feb 18, 2019

Bump plugin version to 5.1.0-rc.1 #13908

Merged

Bump plugin version to 5.1.0 #13960

Merged

psealock mentioned this pull request Mar 5, 2019

Component: date-time: Remove comparison to "now" when testing getMomentDate #14230

Merged

5 tasks

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Datepicker: Allow null value for currentDate on mounting (#12963)

06c564f

* Datepicker: Allow null value for currentDate on mounting * flexible assertion, FTW

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Datepicker: Allow null value for currentDate on mounting (#12963)

6c4e93e

* Datepicker: Allow null value for currentDate on mounting * flexible assertion, FTW

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Datepicker: Allow null value for currentDate on mounting #12963

Datepicker: Allow null value for currentDate on mounting #12963

psealock commented Dec 18, 2018 •

edited

Loading

aduth commented Jan 24, 2019

psealock commented Jan 24, 2019

aduth Jan 25, 2019

aduth Jan 25, 2019

psealock Jan 27, 2019

aduth Jan 28, 2019

psealock commented Jan 30, 2019

aduth Jan 30, 2019

psealock Jan 31, 2019

psealock commented Feb 7, 2019

aduth left a comment

aduth commented Feb 26, 2019

psealock commented Feb 27, 2019

psealock commented Mar 5, 2019

	<DatePicker
	currentDate={ currentDate }
	onChange={ onChange }
	/>

Datepicker: Allow null value for currentDate on mounting #12963

Datepicker: Allow null value for currentDate on mounting #12963

Conversation

psealock commented Dec 18, 2018 • edited Loading

Description

Testing

Screenshots

Types of changes

Checklist:

aduth commented Jan 24, 2019

psealock commented Jan 24, 2019

aduth Jan 25, 2019

Choose a reason for hiding this comment

aduth Jan 25, 2019

Choose a reason for hiding this comment

psealock Jan 27, 2019

Choose a reason for hiding this comment

aduth Jan 28, 2019

Choose a reason for hiding this comment

psealock commented Jan 30, 2019

aduth Jan 30, 2019

Choose a reason for hiding this comment

psealock Jan 31, 2019

Choose a reason for hiding this comment

psealock commented Feb 7, 2019

aduth left a comment

Choose a reason for hiding this comment

aduth commented Feb 26, 2019

psealock commented Feb 27, 2019

psealock commented Mar 5, 2019

psealock commented Dec 18, 2018 •

edited

Loading