feat(timepicker): Added min/max attributes to timepicker. #4019
Conversation
|
That is what |
wesleycho
added
component: timepicker
and removed
component: datepicker
resolution: duplicate
labels
|
I apologize, this is for a different component than I thought - I will review this. |
| function addMinutes( date, minutes ) { | ||
| var dt = new Date( date.getTime() + minutes * 60000 ); | ||
| var newDate = new Date( date.getTime() ).setHours( dt.getHours(), dt.getMinutes() ); | ||
| return new Date( newDate ); |
There was a problem hiding this comment.
Is there any reason why newDate can't just get returned? It should already be an independent reference.
There was a problem hiding this comment.
Date.setHours() returns an int, not a Date for some reason. I returned new Date( newDate ) to convert the int back into a Date.
There was a problem hiding this comment.
Oh, I see now.
It would likely be more efficient to do
var newDate = new Date( date.getTime() + minutes * 60000 );
newDate.setHours( dt.getHours(), dt.getMinutes() );
return newDate;
The dt date existed as a temporary structure to figure out how to modify the existing selected date, but now we can just return the date of interest
|
Thanks for this, this is mostly solid work - I do have two comments that should be addressed before this is merged. |
|
I believe I addressed your concerns. I was able to remove an instance of Date in addMinutes(), slightly different than your suggest because it needs to "rollover" from 11:59PM to 12:00AM on the same day, not the next. I removed the unnecessary conditionals on $attrs.min/max. |
|
|
||
| $scope.noIncrementHours = function() { | ||
| var incrementedSelected = addMinutes( selected, hourStep * 60 ); | ||
| return incrementedSelected > max || (min && incrementedSelected < selected); |
There was a problem hiding this comment.
So what I meant was more return (max && incrementedSelected > max) || incrementedSelected < selected;, and so on.
There was a problem hiding this comment.
The logic is a bit different than that. incrementedSelected > max is catching the common case, increasing the time would exceed the max. If max is undefined, incrementedSelected > max will be false, which is what we want(to allow the increase).
The other part, (min && incrementedSelected < selected) was intended to disable the increment/decrement if the time would rollover(at midnight) if the opposite direction's wouldn't be allowed. For example, if it is 11:30pm, you cannot increase to 12:30am if the minimum is 1:00am.
This code isn't quite there though. This just prevents a rollover if the minimum/maximum is defined. For example, if it is 11:30pm, you cannot increase to 12:30am if the minimum is 12:01am. This should be allowed, as the updated time is still later than the minimum.
I believe the correct code will be return incrementedSelected > max || (incrementedSelected < selected && incrementedSelected < min);. I've manually tested it. I'll write up some unit tests and submit it.
| }; | ||
| $scope.incrementMinutes = function() { | ||
| addMinutes( minuteStep ); | ||
| if(!$scope.noIncrementMinutes()) { |
|
Once these changes are in, this is good to go. |
|
I updated the style, and changed the noIncremenentHours() etc... to disable the time step in a direction if when time rolls over, the result exceeds the limit for movement in the opposite direction. |
|
LGTM - merging shortly with documentation & code style fixes. |
Adds attributes for timepicker's min and max time.