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

Components: Fix the timepicker, only update the values on blur #4193

Merged
merged 1 commit into from Jan 5, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Dec 28, 2017

closes #3661

This PR changes the behavior of the TimPicker to be able to edit it freely and do the validation and updates on blur only.

This also makes the Component API more consistent, the format of the input and output value is made consistent. (Moment is only internal while the API expects timezone-less dates)

Testing instructions

  • Check that you can type anything in the time field and it resets on blur
  • Check that you can clear the input and type an hour

@youknowriad youknowriad added the Chrome label Dec 28, 2017

@youknowriad youknowriad self-assigned this Dec 28, 2017

const { onChange } = this.props;
const { am, date, hours } = this.state;
if ( am === value ) {
return;

This comment has been minimized.

@chriskmnds

chriskmnds Jan 2, 2018

Contributor

Does this ever fire, or is it just for sanity here?

@chriskmnds

chriskmnds Jan 2, 2018

Contributor

Does this ever fire, or is it just for sanity here?

This comment has been minimized.

@youknowriad

youknowriad Jan 3, 2018

Contributor

It does fire when you hit the "toggled" button

@youknowriad

youknowriad Jan 3, 2018

Contributor

It does fire when you hit the "toggled" button

This comment has been minimized.

@chriskmnds

chriskmnds Jan 3, 2018

Contributor

Ah cool, thanks for clarifying. It wasn't clear to me, and it's not covered by the tests either, so I thought to ask :-)

@chriskmnds

chriskmnds Jan 3, 2018

Contributor

Ah cool, thanks for clarifying. It wasn't clear to me, and it's not covered by the tests either, so I thought to ask :-)

/**
* Module Constants
*/
const TIMEZONELESS_FORMAT = 'YYYY-MM-DDTHH:mm:ss';

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

Why not just TIME_FORMAT?

@mtias

mtias Jan 3, 2018

Contributor

Why not just TIME_FORMAT?

This comment has been minimized.

@youknowriad

youknowriad Jan 3, 2018

Contributor

The input and the output format should be the same, that's the format we have in the post object. We could use only a time format for time and date format for date but this means adding some "unnecessary" date manipulations in the parent component date-time (extract time and date on input, and recompute the whole date on change). I think it's just for convenience.

@youknowriad

youknowriad Jan 3, 2018

Contributor

The input and the output format should be the same, that's the format we have in the post object. We could use only a time format for time and date format for date but this means adding some "unnecessary" date manipulations in the parent component date-time (extract time and date on input, and recompute the whole date on change). I think it's just for convenience.

isToggled={ am === 'AM' }
onClick={ this.updateAmPm( 'AM' ) }
>
{ __( 'AM' ) }

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

Not sure if these need some translation context in general. cc @yoavf

@mtias

mtias Jan 3, 2018

Contributor

Not sure if these need some translation context in general. cc @yoavf

This comment has been minimized.

@aduth

aduth Jan 3, 2018

Member

Not sure if these need some translation context in general.

I would have possibly expected context to be good here, but noting elsewhere in core it is not provided:

https://github.com/WordPress/wordpress-develop/blob/f3d3254/src/wp-includes/class-wp-locale.php#L197-L201

@aduth

aduth Jan 3, 2018

Member

Not sure if these need some translation context in general.

I would have possibly expected context to be good here, but noting elsewhere in core it is not provided:

https://github.com/WordPress/wordpress-develop/blob/f3d3254/src/wp-includes/class-wp-locale.php#L197-L201

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

Going to merge this if no more concerns?

Contributor

youknowriad commented Jan 5, 2018

Going to merge this if no more concerns?

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 5, 2018

Contributor

🚢

Contributor

mtias commented Jan 5, 2018

🚢

@youknowriad youknowriad merged commit 74e95a9 into master Jan 5, 2018

3 checks passed

codecov/project 39.69% (+0.53%) compared to 003d533
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/time-picker branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment