post-schedule:clock: janitorial #10540

Merged
merged 2 commits into from Jan 16, 2017
@retrofox
Contributor
retrofox commented Jan 10, 2017 edited
  • It just fix some eslint warnings using const and/or let instead of var, do not bind function into the properties, do not use arrow functions either, etc.
  • use localize helper
  • destruct this.props object

Testing

1 - Go to devdocs page of PostSchedule: http://calypso.localhost:3000/devdocs/blocks/post-schedule

2 - Play into the clock changing hours and minutes of the time.

Everything should be ok.

post-schedule-clock

Also, I've added unit tests. To run them:

> npm run test-client client/components/post-schedule/test/index.js 

Live testing branch

https://calypso.live/devdocs/blocks/post-schedule?branch=improve/post-schedule-clock

@retrofox retrofox requested a review from gwwar Jan 10, 2017
@retrofox retrofox requested a review from obenland Jan 11, 2017
@@ -38,17 +38,24 @@ function checkTimeValue( value ) {
}
class PostScheduleClock extends Component {
+ constructor( props ) {
+ super( props );
+
@obenland
obenland Jan 11, 2017 Member

One of the things I learned from @aduth is the following pattern that keeps the class dumb to the arguments that it's being passed if they're not needed.

class PostScheduleClock extends Component {
    constructor() {
        super( ...arguments );
        // ...
    }

    // ...
}

It doesn't make a huge difference but I like it's simplicity

+ super( props );
+
+ // bounds
+ this.setTime = this.setTime.bind( this );
@obenland
obenland Jan 11, 2017 Member

For this one method you could use Stage 2 class property initializers:

class PostScheduleClock extends Component {
    setTime = () => {
        // ...
    };

    // ...
}

More food for thought than anything else

@gwwar
gwwar Jan 12, 2017 Member

I prefer this since we can usually avoid the need for creating a custom constructor.

+ super( ...arguments );
+
+ // bounds
+ this.adjustHour = this.handleKeyDown.bind( this, 'hour' );
@omarjackman
omarjackman Jan 12, 2017 Contributor

This might be a good place to use some es6 instead of bind

this.adjustHour = event => this.handleKeyDown( 'hour', event );
this.adjustMinute = event => this.handleKeyDown( 'minute', event );
@@ -23,10 +23,10 @@ const noop = () => {};
/**
* Check if the given value is useful to use in time format
* @param {String} value - time value to check
- * @return {String} checked value
+ * @return {String} checked value or `false`
@gwwar
gwwar Jan 12, 2017 Member

false as a boolean? or the string 'false'

@gwwar
gwwar Jan 12, 2017 Member

Let's update the JSDoc type then:

@return {String|Boolean}
@@ -23,10 +23,10 @@ const noop = () => {};
/**
* Check if the given value is useful to use in time format
* @param {String} value - time value to check
- * @return {String} checked value
+ * @return {String} checked value or `false`
*/
function checkTimeValue( value ) {
@gwwar
gwwar Jan 12, 2017 Member

Maybe a better name would be parseAndValidateHourOrMinute

@gwwar
gwwar Jan 12, 2017 Member

Bonus points for adding unit tests to describe the cases this catches.

@retrofox
retrofox Jan 13, 2017 Contributor

Added in 5875bfa
I'm afraid I'll need an aditional review 😞

@retrofox
retrofox Jan 13, 2017 Contributor
> npm run test-client client/components/post-schedule/test/index.js 
@gwwar
Member
gwwar commented Jan 12, 2017

Left some minor notes but feel free to 🚢

+ return date;
+};
+
+const convertMinutesToHHMM = minutes => {
@gwwar
gwwar Jan 13, 2017 Member

@retrofox do we need this? Isn't this equivalent to:

moment().format( 'hh:mm' )
@retrofox
retrofox Jan 16, 2017 Contributor

The helper function expects some minutes instead of a Date (Moment) object. But we could create a temporal instance just to represent the part of hh:mm. We should tweak it to handle the negative signal, though.

const convertMinutesToHHMM = ( minutes = 0 ) => {
	const tempoDate = moment( '2000-1-1').minutes( Math.abs( minutes ) );
	return ( minutes > 0 ? '' : '-' ) + tempoDate.format( 'hh:mm' );
}
@retrofox
retrofox Jan 16, 2017 edited Contributor

Actually we will have to handle other edge-cases:

convertMinutesToHHMM( 100 );  // -> `01:40` ok!
convertMinutesToHHMM( -100 );  // -> `-01:40` ok!
convertMinutesToHHMM( 60 );  // -> `01:00` I'd like get `1`
convertMinutesToHHMM( -60 );  // -> `-01:00` I'd like get `-1` here
convertMinutesToHHMM( 0 );  // -> `-12:00` I'd like get `0`
convertMinutesToHHMM( -0 );  // -> `-12:00` ... and `0`

So I guess we should keep using the current implementation.

+};
+
+const convertMinutesToHHMM = minutes => {
+ const hours = Math.trunc( minutes / 60 );
@gwwar
gwwar Jan 13, 2017 Member

Careful, Math.trunc has no support in IE. Let's remember to lookup browser support when using native methods.

@aduth
aduth Jan 16, 2017 Member

I believe this should be polyfilled for us? Or at least it's been my understanding is that we have polyfills for all non-prototype methods on the base type objects (see Object.assign).

@retrofox
retrofox Jan 16, 2017 Contributor

If I'm not wrong, @aduth isn't wrong. :-o. We are polyfilling using core-js. Here is the defined Math.trunc method.

@retrofox
retrofox Jan 16, 2017 Contributor

Tested under E11:

@gwwar
Member
gwwar commented Jan 13, 2017

Thanks for adding tests @retrofox ! Let's avoid adding new util functions until they're used in code.

But everything else looks ready to 🚢

@retrofox
Contributor

Let's avoid adding new util functions until they're used in code.

They have been there for a while. I've simply just added some unit tests for some of them. :-D

@gwwar
Member
gwwar commented Jan 16, 2017

They have been there for a while. I've simply just added some unit tests for some of them. :-D

Oh I must have missed them in the usage search. If that's the case, go ahead and :shipit:

@retrofox retrofox post-schedule:clock: Add unit tests
b3fcc03
@retrofox retrofox merged commit c5d9095 into master Jan 16, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/i18n 0 new strings. ¡Ándale!
Details
@retrofox
Contributor

Thanks devs for the review.

@lancewillett lancewillett deleted the improve/post-schedule-clock branch Jan 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment