From 3d1b4366562f5e6081edfbebfbabbb16a782e361 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 4 Aug 2020 10:13:07 +0800 Subject: [PATCH 1/5] Refactor tests to use RTL --- .../date-time/test/__snapshots__/time.js.snap | 689 ------------------ .../components/src/date-time/test/time.js | 242 +++--- 2 files changed, 123 insertions(+), 808 deletions(-) delete mode 100644 packages/components/src/date-time/test/__snapshots__/time.js.snap diff --git a/packages/components/src/date-time/test/__snapshots__/time.js.snap b/packages/components/src/date-time/test/__snapshots__/time.js.snap deleted file mode 100644 index ae17692f74a5fb..00000000000000 --- a/packages/components/src/date-time/test/__snapshots__/time.js.snap +++ /dev/null @@ -1,689 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`TimePicker matches the snapshot when the is12hour prop is false 1`] = ` -
-
- - Date - -
-
- -
-
- -
-
- -
-
-
-
- - Time - -
-
- - - -
- -
-
-
-`; - -exports[`TimePicker matches the snapshot when the is12hour prop is specified 1`] = ` -
-
- - Date - -
-
- -
-
- -
-
- -
-
-
-
- - Time - -
-
- - - -
- - - AM - - - PM - - - -
-
-
-`; - -exports[`TimePicker matches the snapshot when the is12hour prop is true 1`] = ` -
-
- - Date - -
-
- -
-
- -
-
- -
-
-
-
- - Time - -
-
- - - -
- - - AM - - - PM - - - -
-
-
-`; - -exports[`TimePicker matches the snapshot when the is12hour prop is undefined 1`] = ` -
-
- - Date - -
-
- -
-
- -
-
- -
-
-
-
- - Time - -
-
- - - -
- -
-
-
-`; diff --git a/packages/components/src/date-time/test/time.js b/packages/components/src/date-time/test/time.js index c55ef191275090..699f9f6e0bbef2 100644 --- a/packages/components/src/date-time/test/time.js +++ b/packages/components/src/date-time/test/time.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { shallow } from 'enzyme'; +import { render, fireEvent, screen } from '@testing-library/react'; /** * Internal dependencies @@ -9,202 +9,170 @@ import { shallow } from 'enzyme'; import TimePicker from '../time'; describe( 'TimePicker', () => { - it( 'matches the snapshot when the is12hour prop is true', () => { - const wrapper = shallow( - - ); - expect( wrapper ).toMatchSnapshot(); - } ); - - it( 'matches the snapshot when the is12hour prop is false', () => { - const wrapper = shallow( - - ); - expect( wrapper ).toMatchSnapshot(); - } ); - - it( 'matches the snapshot when the is12hour prop is specified', () => { - const wrapper = shallow( - - ); - expect( wrapper ).toMatchSnapshot(); - } ); - - it( 'matches the snapshot when the is12hour prop is undefined', () => { - const wrapper = shallow( - - ); - expect( wrapper ).toMatchSnapshot(); - } ); - - it( 'should call onChange with an updated day', () => { + it( 'should call onChange with updated date values', () => { const onChangeSpy = jest.fn(); - const picker = shallow( - - ); - const input = picker - .find( '.components-datetime__time-field-day-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '10' } } ); - input.simulate( 'blur' ); - expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-10T11:00:00' ); - } ); - it( 'should call onChange with an updated month', () => { - const onChangeSpy = jest.fn(); - const picker = shallow( + render( ); - const input = picker - .find( '.components-datetime__time-field-month-select' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '12' } } ); - input.simulate( 'blur' ); + + const monthInput = screen.getByLabelText( 'Month' ); + const dayInput = screen.getByLabelText( 'Day' ); + const yearInput = screen.getByLabelText( 'Year' ); + const hoursInput = screen.getByLabelText( 'Hours' ); + const minutesInput = screen.getByLabelText( 'Minutes' ); + + fireEvent.change( monthInput, { target: { value: '12' } } ); + fireEvent.blur( monthInput ); + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-12-18T11:00:00' ); - } ); + onChangeSpy.mockClear(); - it( 'should call onChange with an updated year', () => { - const onChangeSpy = jest.fn(); - const picker = shallow( - - ); - const input = picker - .find( '.components-datetime__time-field-year-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '2018' } } ); - input.simulate( 'blur' ); - expect( onChangeSpy ).toHaveBeenCalledWith( '2018-10-18T11:00:00' ); + fireEvent.change( dayInput, { target: { value: '22' } } ); + fireEvent.blur( dayInput ); + + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-12-22T11:00:00' ); + onChangeSpy.mockClear(); + + fireEvent.change( yearInput, { target: { value: '2018' } } ); + fireEvent.blur( yearInput ); + + expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T11:00:00' ); + onChangeSpy.mockClear(); + + fireEvent.change( hoursInput, { target: { value: '12' } } ); + fireEvent.blur( hoursInput ); + + expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T12:00:00' ); + onChangeSpy.mockClear(); + + fireEvent.change( minutesInput, { target: { value: '35' } } ); + fireEvent.blur( minutesInput ); + + expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T12:35:00' ); + onChangeSpy.mockClear(); } ); it( 'should call onChange with an updated hour (12-hour clock)', () => { const onChangeSpy = jest.fn(); - const picker = shallow( + + render( ); - const input = picker - .find( '.components-datetime__time-field-hours-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '10' } } ); - input.simulate( 'blur' ); + + const hoursInput = screen.getByLabelText( 'Hours' ); + + fireEvent.change( hoursInput, { target: { value: '10' } } ); + fireEvent.blur( hoursInput ); + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T10:00:00' ); } ); it( 'should not call onChange with an updated hour (12-hour clock) if the hour is out of bounds', () => { const onChangeSpy = jest.fn(); - const picker = shallow( + + render( ); - const input = picker - .find( '.components-datetime__time-field-hours-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '22' } } ); - input.simulate( 'blur' ); + + const hoursInput = screen.getByLabelText( 'Hours' ); + + fireEvent.change( hoursInput, { target: { value: '22' } } ); + fireEvent.blur( hoursInput ); + expect( onChangeSpy ).not.toHaveBeenCalled(); } ); it( 'should call onChange with an updated hour (24-hour clock)', () => { const onChangeSpy = jest.fn(); - const picker = shallow( + + render( ); - const input = picker - .find( '.components-datetime__time-field-hours-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '22' } } ); - input.simulate( 'blur' ); - expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T22:00:00' ); - } ); - it( 'should call onChange with an updated minute', () => { - const onChangeSpy = jest.fn(); - const picker = shallow( - - ); - const input = picker - .find( '.components-datetime__time-field-minutes-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '10' } } ); - input.simulate( 'blur' ); - expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T11:10:00' ); + const hoursInput = screen.getByLabelText( 'Hours' ); + + fireEvent.change( hoursInput, { target: { value: '22' } } ); + fireEvent.blur( hoursInput ); + + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T22:00:00' ); } ); it( 'should not call onChange with an updated minute if out of bounds', () => { const onChangeSpy = jest.fn(); - const picker = shallow( + + render( ); - const input = picker - .find( '.components-datetime__time-field-minutes-input' ) - .at( 0 ); - input.simulate( 'change', { target: { value: '99' } } ); - input.simulate( 'blur' ); + + const minutesInput = screen.getByLabelText( 'Minutes' ); + + fireEvent.change( minutesInput, { target: { value: '99' } } ); + fireEvent.blur( minutesInput ); + expect( onChangeSpy ).not.toHaveBeenCalled(); } ); it( 'should switch to PM correctly', () => { const onChangeSpy = jest.fn(); - const button = shallow( + + render( ); - const pmButton = button.find( '.components-datetime__time-pm-button' ); - pmButton.simulate( 'click' ); + + const pmButton = screen.getByText( 'PM' ); + + fireEvent.click( pmButton ); + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T23:00:00' ); } ); it( 'should switch to AM correctly', () => { const onChangeSpy = jest.fn(); - const button = shallow( + + render( ); - const pmButton = button.find( '.components-datetime__time-am-button' ); - pmButton.simulate( 'click' ); + + const amButton = screen.getByText( 'AM' ); + + fireEvent.click( amButton ); + expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T11:00:00' ); } ); it( 'should truncate at the minutes on change', () => { const onChangeSpy = jest.fn(); - const wrapper = shallow( + render( { /> ); - const minuteInput = wrapper.find( 'input[aria-label="Minutes"]' ); + const minutesInput = screen.getByLabelText( 'Minutes' ); - minuteInput.simulate( 'change', { target: { value: '22' } } ); - minuteInput.simulate( 'blur' ); + fireEvent.change( minutesInput, { target: { value: '22' } } ); + fireEvent.blur( minutesInput ); expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T23:22:00' ); } ); + + it( 'should reset the date when currentTime changed', () => { + const onChangeSpy = jest.fn(); + + const { rerender } = render( + + ); + + rerender( + + ); + + expect( screen.getByLabelText( 'Month' ).value ).toBe( '07' ); + expect( screen.getByLabelText( 'Day' ).value ).toBe( '13' ); + expect( screen.getByLabelText( 'Year' ).value ).toBe( '2020' ); + expect( screen.getByLabelText( 'Hours' ).value ).toBe( '06' ); + expect( screen.getByLabelText( 'Minutes' ).value ).toBe( '00' ); + /** + * This is not ideal, but best of we can do for now until we refactor + * AM/PM into accessible elements, like radio buttons. + */ + expect( + screen.getByText( 'AM' ).classList.contains( 'is-primary' ) + ).toBe( false ); + expect( + screen.getByText( 'PM' ).classList.contains( 'is-primary' ) + ).toBe( true ); + } ); } ); From a61dd4988df9ad7297f4cbd30b5f4eb6aa8627d0 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 4 Aug 2020 12:52:31 +0800 Subject: [PATCH 2/5] Add test for ordering of the month/day input --- .../components/src/date-time/test/time.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/packages/components/src/date-time/test/time.js b/packages/components/src/date-time/test/time.js index 699f9f6e0bbef2..32a47ca9ed2dc6 100644 --- a/packages/components/src/date-time/test/time.js +++ b/packages/components/src/date-time/test/time.js @@ -223,4 +223,52 @@ describe( 'TimePicker', () => { screen.getByText( 'PM' ).classList.contains( 'is-primary' ) ).toBe( true ); } ); + + it( 'should have different layouts/orders for 12/24 hour formats', () => { + const onChangeSpy = jest.fn(); + + const { rerender } = render( +
+ + + ); + + const form = screen.getByRole( 'form' ); + + let monthInputIndex = [].indexOf.call( + form.elements, + screen.getByLabelText( 'Month' ) + ); + let dayInputIndex = [].indexOf.call( + form.elements, + screen.getByLabelText( 'Day' ) + ); + + expect( monthInputIndex < dayInputIndex ).toBe( true ); + + rerender( +
+ + + ); + + monthInputIndex = [].indexOf.call( + form.elements, + screen.getByLabelText( 'Month' ) + ); + dayInputIndex = [].indexOf.call( + form.elements, + screen.getByLabelText( 'Day' ) + ); + + expect( monthInputIndex > dayInputIndex ).toBe( true ); + } ); } ); From 21b8bebe2be56b83c4f408897af298ae2cbf2e76 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Tue, 4 Aug 2020 15:33:08 +0800 Subject: [PATCH 3/5] Add storybook for --- .../components/src/date-time/stories/time.js | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 packages/components/src/date-time/stories/time.js diff --git a/packages/components/src/date-time/stories/time.js b/packages/components/src/date-time/stories/time.js new file mode 100644 index 00000000000000..52ed6fb7ce8329 --- /dev/null +++ b/packages/components/src/date-time/stories/time.js @@ -0,0 +1,26 @@ +/** + * Internal dependencies + */ +import TimePicker from '../time'; + +/** + * External dependencies + */ +import { date, boolean } from '@storybook/addon-knobs'; +import { noop } from 'lodash'; + +export default { title: 'Components/TimePicker', component: TimePicker }; + +export const _default = () => { + return ( + + ); +}; From ba0bdbf80ad01158a832723ffdc01832a7545f67 Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Fri, 31 Jul 2020 15:06:00 +0800 Subject: [PATCH 4/5] Refactor --- packages/components/src/date-time/time.js | 532 +++++++++------------- 1 file changed, 224 insertions(+), 308 deletions(-) diff --git a/packages/components/src/date-time/time.js b/packages/components/src/date-time/time.js index 1b502d70aaf669..491e2ec884ef4c 100644 --- a/packages/components/src/date-time/time.js +++ b/packages/components/src/date-time/time.js @@ -8,7 +8,12 @@ import moment from 'moment'; /** * WordPress dependencies */ -import { Component } from '@wordpress/element'; +import { + createElement, + useState, + useMemo, + useEffect, +} from '@wordpress/element'; import { __ } from '@wordpress/i18n'; /** @@ -23,347 +28,258 @@ import TimeZone from './timezone'; */ const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss'; -class TimePicker extends Component { - constructor() { - super( ...arguments ); - this.state = { - day: '', - month: '', - year: '', - hours: '', - minutes: '', - am: '', - date: null, - }; - this.changeDate = this.changeDate.bind( this ); - this.updateMonth = this.updateMonth.bind( this ); - this.onChangeMonth = this.onChangeMonth.bind( this ); - this.updateDay = this.updateDay.bind( this ); - this.onChangeDay = this.onChangeDay.bind( this ); - this.updateYear = this.updateYear.bind( this ); - this.onChangeYear = this.onChangeYear.bind( this ); - this.updateHours = this.updateHours.bind( this ); - this.updateMinutes = this.updateMinutes.bind( this ); - this.onChangeHours = this.onChangeHours.bind( this ); - this.onChangeMinutes = this.onChangeMinutes.bind( this ); - this.renderMonth = this.renderMonth.bind( this ); - this.renderDay = this.renderDay.bind( this ); - this.renderDayMonthFormat = this.renderDayMonthFormat.bind( this ); - } +/** + * + * A shared component to parse, validate, and handle remounting of the underlying element + * + * @param {Object} props Component props + * @param {string} props.as Render the component as specific element tag, defaults to "input" + * @param {number | string} props.value The default value of the component + * @param {Function} props.onUpdate Call back when blurred and validated + */ +function NumberInput( { as, value, onUpdate, ...props } ) { + function handleBlur( event ) { + const { target } = event; - componentDidMount() { - this.syncState( this.props ); - } + if ( value === target.value ) { + return; + } - componentDidUpdate( prevProps ) { - const { currentTime, is12Hour } = this.props; + const parsedValue = parseInt( target.value, 10 ); + + // Run basic number validation on the input if ( - currentTime !== prevProps.currentTime || - is12Hour !== prevProps.is12Hour + ! isInteger( parsedValue ) || + ( typeof props.max !== 'undefined' && parsedValue > props.max ) || + ( typeof props.min !== 'undefined' && parsedValue < props.min ) ) { - this.syncState( this.props ); + // If validation failed, reset the value to the previous valid value + target.value = value; + } else { + // Otherwise, it's valid, call onUpdate + onUpdate( target.name, parsedValue ); } } + + return createElement( as || 'input', { + // Re-mount the input value to accept the latest value as the defaultValue + key: value, + defaultValue: value, + onBlur: handleBlur, + ...props, + } ); +} + +/** + * + * + * @param {Object} props Component props + * @param {boolean} props.is12Hour Should the time picker showed in 12 hour format or 24 hour format + * @param {Date | string | number} props.currentTime The initial current time the time picker should render + * @param {Function} props.onChange Callback function when the date changed + */ +export function TimePicker( { is12Hour, currentTime, onChange } ) { + const [ date, setDate ] = useState( () => + // Truncate the date at the minutes, see: #15495 + moment( currentTime ).startOf( 'minutes' ) + ); + + // Reset the state when currentTime changed + useEffect( () => { + setDate( moment( currentTime ).startOf( 'minutes' ) ); + }, [ currentTime ] ); + + const { day, month, year, minutes, hours, am } = useMemo( + () => ( { + day: date.format( 'DD' ), + month: date.format( 'MM' ), + year: date.format( 'YYYY' ), + minutes: date.format( 'mm' ), + hours: date.format( is12Hour ? 'hh' : 'HH' ), + am: date.format( 'H' ) <= 11 ? 'AM' : 'PM', + } ), + [ date, is12Hour ] + ); + /** * Function that sets the date state and calls the onChange with a new date. * The date is truncated at the minutes. * * @param {Object} newDate The date object. */ - changeDate( newDate ) { - const dateWithStartOfMinutes = newDate.clone().startOf( 'minute' ); - this.setState( { date: dateWithStartOfMinutes } ); - this.props.onChange( - dateWithStartOfMinutes.format( TIMEZONELESS_FORMAT ) - ); - } - - getMaxHours() { - return this.props.is12Hour ? 12 : 23; + function changeDate( newDate ) { + setDate( newDate ); + onChange( newDate.format( TIMEZONELESS_FORMAT ) ); } - getMinHours() { - return this.props.is12Hour ? 1 : 0; + function update( name, value ) { + // Clone the date and call the specific setter function according to `name` + const newDate = date.clone()[ name ]( value ); + changeDate( newDate ); } - syncState( { currentTime, is12Hour } ) { - const selected = currentTime ? moment( currentTime ) : moment(); - const day = selected.format( 'DD' ); - const month = selected.format( 'MM' ); - const year = selected.format( 'YYYY' ); - const minutes = selected.format( 'mm' ); - const am = selected.format( 'H' ) <= 11 ? 'AM' : 'PM'; - const hours = selected.format( is12Hour ? 'hh' : 'HH' ); - const date = currentTime ? moment( currentTime ) : moment(); - this.setState( { day, month, year, minutes, hours, am, date } ); - } - - updateHours() { - const { is12Hour } = this.props; - const { am, hours, date } = this.state; - const value = parseInt( hours, 10 ); - if ( value === date.hour() ) { - return; - } - if ( - ! isInteger( value ) || - ( is12Hour && ( value < 1 || value > 12 ) ) || - ( ! is12Hour && ( value < 0 || value > 23 ) ) - ) { - this.syncState( this.props ); - return; - } - - const newDate = is12Hour - ? date - .clone() - .hours( - am === 'AM' ? value % 12 : ( ( value % 12 ) + 12 ) % 24 - ) - : date.clone().hours( value ); - this.changeDate( newDate ); - } - - updateMinutes() { - const { minutes, date } = this.state; - const value = parseInt( minutes, 10 ); - if ( value === date.minute() ) { - return; - } - if ( ! isInteger( value ) || value < 0 || value > 59 ) { - this.syncState( this.props ); - return; - } - const newDate = date.clone().minutes( value ); - this.changeDate( newDate ); - } - - updateDay() { - const { day, date } = this.state; - const value = parseInt( day, 10 ); - if ( value === date.date() ) { - return; - } - if ( ! isInteger( value ) || value < 1 || value > 31 ) { - this.syncState( this.props ); - return; - } - const newDate = date.clone().date( value ); - this.changeDate( newDate ); - } - - updateMonth() { - const { month, date } = this.state; - const value = parseInt( month, 10 ); - if ( value === date.month() + 1 ) { - return; - } - if ( ! isInteger( value ) || value < 1 || value > 12 ) { - this.syncState( this.props ); - return; - } - const newDate = date.clone().month( value - 1 ); - this.changeDate( newDate ); - } - - updateYear() { - const { year, date } = this.state; - const value = parseInt( year, 10 ); - if ( value === date.year() ) { - return; - } - if ( ! isInteger( value ) || value < 0 || value > 9999 ) { - this.syncState( this.props ); - return; - } - const newDate = date.clone().year( value ); - this.changeDate( newDate ); - } - - updateAmPm( value ) { + function updateAmPm( value ) { return () => { - const { am, date, hours } = this.state; if ( am === value ) { return; } - let newDate; - if ( value === 'PM' ) { - newDate = date - .clone() - .hours( ( ( parseInt( hours, 10 ) % 12 ) + 12 ) % 24 ); - } else { - newDate = date.clone().hours( parseInt( hours, 10 ) % 12 ); - } - this.changeDate( newDate ); - }; - } - onChangeDay( event ) { - this.setState( { day: event.target.value } ); - } + const parsedHours = parseInt( hours, 10 ); - onChangeMonth( event ) { - this.setState( { month: event.target.value } ); - } + const newDate = date + .clone() + .hours( + value === 'PM' + ? ( ( parsedHours % 12 ) + 12 ) % 24 + : parsedHours % 12 + ); - onChangeYear( event ) { - this.setState( { year: event.target.value } ); - } - - onChangeHours( event ) { - this.setState( { hours: event.target.value } ); + changeDate( newDate ); + }; } - onChangeMinutes( event ) { - const minutes = event.target.value; - this.setState( { - minutes: minutes === '' ? '' : ( '0' + minutes ).slice( -2 ), - } ); - } + const dayFormat = ( +
+ +
+ ); - renderMonth( month ) { - return ( -
+ update( key, value - 1 ) } > - -
- ); - } + + + + + + + + + + + + + + + ); - renderDay( day ) { - return ( -
- -
- ); - } - - renderDayMonthFormat( is12Hour ) { - const { day, month } = this.state; - const layout = [ this.renderDay( day ), this.renderMonth( month ) ]; - return is12Hour ? layout : layout.reverse(); - } + const dayMonthFormat = is12Hour ? ( + <> + { dayFormat } + { monthFormat } + + ) : ( + <> + { monthFormat } + { dayFormat } + + ); - render() { - const { is12Hour } = this.props; - const { year, minutes, hours, am } = this.state; + return ( +
+
+ + { __( 'Date' ) } + +
+ { dayMonthFormat } - return ( -
-
- - { __( 'Date' ) } - -
- { this.renderDayMonthFormat( is12Hour ) } -
- -
+
+
-
+
+
-
- - { __( 'Time' ) } - -
-
- -
-
-
- ); - } + + + + + ); } export default TimePicker; From 74ceba53ef88b8b20582f65b2a6f5cf795e14dfb Mon Sep 17 00:00:00 2001 From: Kai Hao Date: Thu, 13 Aug 2020 14:04:56 +0800 Subject: [PATCH 5/5] Coding style updates as per suggested in code reviews --- packages/components/src/date-time/time.js | 54 ++++++++++++----------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/packages/components/src/date-time/time.js b/packages/components/src/date-time/time.js index 491e2ec884ef4c..5df865e95e27d4 100644 --- a/packages/components/src/date-time/time.js +++ b/packages/components/src/date-time/time.js @@ -29,15 +29,15 @@ import TimeZone from './timezone'; const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss'; /** - * - * A shared component to parse, validate, and handle remounting of the underlying element + * + * A shared component to parse, validate, and handle remounting of the underlying form field element like and