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.

Sign up for GitHub

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

DateTimePicker: set PM hours correctly #36878

Merged
merged 6 commits into from
Nov 28, 2021
Merged

DateTimePicker: set PM hours correctly #36878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
535fd0c
DateTimePicker: Extract `from12hTo24h` function
ciampo Nov 25, 2021
0c815f9
DateTimePicker: correctly convert hours from 12 to 24 format when upd…
ciampo Nov 25, 2021
b6ce3a8
CHANGELOG
ciampo Nov 25, 2021
813684b
Only apply the value adjustment when the hours input updates
ciampo Nov 26, 2021
66c847e
Add unit test to check setting hours with PM period selected
ciampo Nov 26, 2021
3532e49
Update unit test on the assumption that 12am should be intended as mi…
ciampo Nov 26, 2021
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Bug Fix

- Replaced hardcoded blue in `ColorPicker` with UI theme color ([#36153](https://github.com/WordPress/gutenberg/pull/36153)).
- Fixed a bug which prevented setting `PM` hours correctly in the `DateTimePicker` ([#36878](https://github.com/WordPress/gutenberg/pull/36878)).

### Enhancements
- Added a `showTooltip` prop to `ToggleGroupControlOption` in order to display tooltip text (using `<Tooltip />`). ([#36726](https://github.com/WordPress/gutenberg/pull/36726)).
Expand Down
34 changes: 32 additions & 2 deletions packages/components/src/date-time/test/time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ describe( 'TimePicker', () => {
fireEvent.change( hoursInput, { target: { value: '12' } } );
fireEvent.blur( hoursInput );

expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T12:00:00' );
expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T00:00:00' );
onChangeSpy.mockClear();

fireEvent.change( minutesInput, { target: { value: '35' } } );
fireEvent.blur( minutesInput );

expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T12:35:00' );
expect( onChangeSpy ).toHaveBeenCalledWith( '2018-12-22T00:35:00' );
Comment on lines +50 to +56
Copy link
Contributor Author

@ciampo ciampo Nov 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The fix introduced in this PR caused this unit test to fail. After looking more into it, I believe that the unit test was previously wrong:

  • the time picker in the test is using the 12 hours period
  • the initial time set to the picker is 1986-10-18T11:00:00, and therefore the picker automatically picks AM as the period
  • the period is not changed explicitly throughout the test
  • the hours input's value is changed to 12 (remember, the period is still set to am)
  • the test currently expected 12am to be converted to midday
  • the test was passing until now because of the lack of value adjustment prior to this PR

I believe that it is correct to update the unit test in this case, since 12am represents midnight and therefore should be converted to 00 in the 24h format

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, +1 from me!

onChangeSpy.mockClear();
} );

Expand Down Expand Up @@ -169,6 +169,36 @@ describe( 'TimePicker', () => {
expect( onChangeSpy ).toHaveBeenCalledWith( '1986-10-18T11:00:00' );
} );

it( 'should allow to set the time correctly when the PM period is selected', () => {
const onChangeSpy = jest.fn();

render(
<TimePicker
currentTime="1986-10-18T11:00:00"
onChange={ onChangeSpy }
is12Hour
/>
);

const pmButton = screen.getByText( 'PM' );
fireEvent.click( pmButton );

const hoursInput = screen.getByLabelText( 'Hours' );
fireEvent.change( hoursInput, { target: { value: '6' } } );
fireEvent.blur( hoursInput );

// When clicking on 'PM', we expect the time to be 11pm
expect( onChangeSpy ).toHaveBeenNthCalledWith(
1,
'1986-10-18T23:00:00'
);
// When changing the hours to '6', we expect the time to be 6pm
expect( onChangeSpy ).toHaveBeenNthCalledWith(
2,
'1986-10-18T18:00:00'
);
} );

it( 'should truncate at the minutes on change', () => {
const onChangeSpy = jest.fn();

Expand Down
20 changes: 14 additions & 6 deletions packages/components/src/date-time/time.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ import TimeZone from './timezone';
*/
const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss';

function from12hTo24h( hours, isPm ) {
return isPm ? ( ( hours % 12 ) + 12 ) % 24 : hours % 12;
}

/**
* <UpdateOnBlurAsIntegerField>
* A shared component to parse, validate, and handle remounting of the underlying form field element like <input> and <select>.
Expand Down Expand Up @@ -117,8 +121,16 @@ export function TimePicker( { is12Hour, currentTime, onChange } ) {
}

function update( name, value ) {
andrewserong marked this conversation as resolved.
Show resolved Hide resolved
// If the 12-hour format is being used and the 'PM' period is selected, then
// the incoming value (which ranges 1-12) should be increased by 12 to match
// the expected 24-hour format.
let adjustedValue = value;
if ( name === 'hours' && is12Hour ) {
adjustedValue = from12hTo24h( value, am === 'PM' );
}

// Clone the date and call the specific setter function according to `name`.
const newDate = date.clone()[ name ]( value );
const newDate = date.clone()[ name ]( adjustedValue );
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
changeDate( newDate );
}

Expand All @@ -132,11 +144,7 @@ export function TimePicker( { is12Hour, currentTime, onChange } ) {

const newDate = date
.clone()
.hours(
value === 'PM'
? ( ( parsedHours % 12 ) + 12 ) % 24
: parsedHours % 12
);
.hours( from12hTo24h( parsedHours, value === 'PM' ) );

changeDate( newDate );
};
Expand Down