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

Datepicker: Allow null value for currentDate on mounting #12963

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

- `Dropdown` now has a `focusOnMount` prop which is passed directly to the contained `Popover`.
- `DatePicker` has new prop `isInvalidDate` exposing react-dates' `isOutsideRange`.
- `DatePicker` allows `null` as accepted value for `currentDate` prop to signify no date selection.

## 7.0.5 (2019-01-03)

Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/date-time/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,11 @@ The component accepts the following props:

### currentDate

The current date and time at initialization.
The current date and time at initialization. Optionally pass in a `null` value to specify no date is currently selected.

- Type: `string`
- Required: Yes
- Required: No
- Default: today's date

### onChange

Expand Down
19 changes: 17 additions & 2 deletions packages/components/src/date-time/date.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ class DatePicker extends Component {
onChangeMoment( newDate ) {
const { currentDate, onChange } = this.props;

// If currentDate is null, use now as momentTime to designate hours, minutes, seconds.
const momentDate = currentDate ? moment( currentDate ) : moment();
const momentTime = {
hours: momentDate.hours(),
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Expand All @@ -35,10 +36,24 @@ class DatePicker extends Component {
onChange( newDate.set( momentTime ).format( TIMEZONELESS_FORMAT ) );
}

/**
* Create a Moment object from a date string. With no currentDate supplied, default to a Moment
* object representing now. If a null value is passed, return a null value.
*
* @param {?string} currentDate Date representing the currently selected date or null to signify no selection.
* @return {?Moment} Moment object for selected date or null.
*/
getMomentDate( currentDate ) {
if ( null === currentDate ) {
return null;
}
return currentDate ? moment( currentDate ) : moment();
}

render() {
const { currentDate, isInvalidDate } = this.props;

const momentDate = currentDate ? moment( currentDate ) : moment();
const momentDate = this.getMomentDate( currentDate );

return (
<div className="components-datetime__date">
Expand All @@ -49,7 +64,7 @@ class DatePicker extends Component {
hideKeyboardShortcutsPanel
// This is a hack to force the calendar to update on month or year change
// https://github.com/airbnb/react-dates/issues/240#issuecomment-361776665
key={ `datepicker-controller-${ momentDate.format( 'MM-YYYY' ) }` }
key={ `datepicker-controller-${ momentDate ? momentDate.format( 'MM-YYYY' ) : 'null' }` }
noBorder
numberOfMonths={ 1 }
onDateChange={ this.onChangeMoment }
Expand Down
110 changes: 110 additions & 0 deletions packages/components/src/date-time/test/date.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
/**
* External dependencies
*/
import { shallow } from 'enzyme';
import moment from 'moment';

/**
* Internal dependencies
*/
import DatePicker from '../date';

const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss';

describe( 'DatePicker', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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.

it( 'should pass down a moment object for currentDate', () => {
const currentDate = '1986-10-18T23:00:00';
const wrapper = shallow( <DatePicker currentDate={ currentDate } /> );
const date = wrapper.children().props().date;
expect( moment.isMoment( date ) ).toBe( true );
expect( date.isSame( moment( currentDate ) ) ).toBe( true );
} );

it( 'should pass down a null date when currentDate is set to null', () => {
const wrapper = shallow( <DatePicker currentDate={ null } /> );
expect( wrapper.children().props().date ).toBeNull();
} );

it( 'should pass down a moment object for now when currentDate is undefined', () => {
const wrapper = shallow( <DatePicker /> );
const date = wrapper.children().props().date;
expect( moment.isMoment( date ) ).toBe( true );
expect( date.isSame( moment(), 'second' ) ).toBe( true );
} );

describe( 'getMomentDate', () => {
it( 'should return a Moment object representing a given date string', () => {
const currentDate = '1986-10-18T23:00:00';
const wrapper = shallow( <DatePicker /> );
const momentDate = wrapper.instance().getMomentDate( currentDate );

expect( moment.isMoment( momentDate ) ).toBe( true );
expect( momentDate.isSame( moment( currentDate ) ) ).toBe( true );
} );

it( 'should return null when given a null agrument', () => {
const currentDate = null;
const wrapper = shallow( <DatePicker /> );
const momentDate = wrapper.instance().getMomentDate( currentDate );

expect( momentDate ).toBeNull();
} );

it( 'should return a Moment object representing now when given an undefined argument', () => {
const wrapper = shallow( <DatePicker /> );
const momentDate = wrapper.instance().getMomentDate();

expect( moment.isMoment( momentDate ) ).toBe( true );
expect( momentDate.isSame( moment(), 'second' ) ).toBe( true );
} );
} );

describe( 'onChangeMoment', () => {
it( 'should call onChange with a formated date of the input', () => {
const onChangeSpy = jest.fn();
const currentDate = '1986-10-18T11:00:00';
const wrapper = shallow( <DatePicker currentDate={ currentDate } onChange={ onChangeSpy } /> );
const newDate = moment();

wrapper.instance().onChangeMoment( newDate );

expect( onChangeSpy ).toHaveBeenCalledWith( newDate.format( TIMEZONELESS_FORMAT ) );
} );

it( 'should call onChange with hours, minutes, seconds of the current time when currentDate is undefined', () => {
let onChangeSpyArgument;
const onChangeSpy = ( arg ) => onChangeSpyArgument = arg;
const wrapper = shallow( <DatePicker onChange={ onChangeSpy } /> );
const newDate = moment( '1986-10-18T11:00:00' );
const current = moment();
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

const newDateWithCurrentTime = newDate
.clone()
.set( {
hours: current.hours(),
minutes: current.minutes(),
seconds: current.seconds(),
} );
wrapper.instance().onChangeMoment( newDate );

expect( moment( onChangeSpyArgument ).isSame( newDateWithCurrentTime, 'minute' ) ).toBe( true );
} );

it( 'should call onChange with hours, minutes, seconds of the current time when currentDate is null', () => {
let onChangeSpyArgument;
const onChangeSpy = ( arg ) => onChangeSpyArgument = arg;
const wrapper = shallow( <DatePicker currentDate={ null } onChange={ onChangeSpy } /> );
const newDate = moment( '1986-10-18T11:00:00' );
const current = moment();
const newDateWithCurrentTime = newDate
.clone()
.set( {
hours: current.hours(),
minutes: current.minutes(),
seconds: current.seconds(),
} );
wrapper.instance().onChangeMoment( newDate );

expect( moment( onChangeSpyArgument ).isSame( newDateWithCurrentTime, 'minute' ) ).toBe( true );
} );
} );
} );