When a date is selected in absolute mode, set to start/end of day #10433
Conversation
| if (_.isDate(date)) $scope.absolute.from = moment(date); | ||
| // If it is a date, it was set from the timepicker, so we set it to the start of day and transform it into a moment | ||
| if (_.isDate(date)) { | ||
| date.setHours(0, 0, 0, 0); // Start of day |
There was a problem hiding this comment.
Instead of having to guess where the update came from, what if we used a getterSetter for the ng-model on the datepicker using ngModelOptions? Then this setHours logic could live inside the setter.
There was a problem hiding this comment.
I had this thought too, but I don't think the version of the date picker we're using supports ngModelOptions. I guess I should look into upgrading the library.
There was a problem hiding this comment.
Does it need to do anything special to support it? I thought the getterSetter would be declared in timepicker.js and it would work without any modification to datepicker, since it would look like a regular model from datepicker's point of view.
| expect($scope.absolute.to.valueOf()).to.be(moment('2012-02-11 23:59:59.999').valueOf()); | ||
| done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Could you add a test ensuring that manually setting hour/minute/second still works?
|
@cjcenizal What are your thoughts on this change? |
|
This makes sense to me. If I'm selecting a date range, I want to see all data that falls within the entire range -- not have a few hours worth excluded from either end. If users, for some reason, start asking for the ability to specify and "lock" the particular hour/minute/second values as they change the start and end days, I suggest we add timepicker controls above the date controls to give them that ability. @alexfrancoeur Any thoughts on this UX? |
|
@cjcenizal this looks good to me. The current date selection is pretty awkward. It keeps the relative timerange and applies the the day/month/year to it. I certainly think this works. The only confusion I could foresee is that the defaults are not consistent (for example, both 00:00:000). That being said, as someone who is investigating this data there is a good chance I'd default to the beginning of the first day and end of the last (or the beginning the last day +1). I think this is a good approach all around. ++ guys! |
|
@Bargs Could you take another look at this? I've used the I had thought that |
|
@lukasolson this is first thing on my todo list tomorrow |
| @@ -421,6 +421,33 @@ describe('timepicker directive', function () { | |||
| done(); | |||
| }); | |||
|
|
|||
| }); | |||
| it('should set from/to to start/end of day if set from timepicker', function (done) { | |||
There was a problem hiding this comment.
Should "timepicker" be "datepicker" in this test description and the one below?
…astic#10433) * In absolute timepicker, use whole days * Use ngModelOptions instead of implicit bindings * fix absolute timepicker test * Fix wording in test
|
Backported to 5.x (5.4) in 4f49a71. |
Prior to this PR, when you would open the timepicker and click on "Absolute", then select a from/to date, the hours/mins/secs/ms of the date would persist as you selected different dates.
This PR changes this behavior so that when you choose a "from" date, it automatically selects the beginning of the date (00:00:00.000), and when you choose a "to" date, it automatically selects the end of the date (23:59:59.999).
Closes #3803.