feat(timepicker): add timepicker directive #358

bekos · 2013-04-26T13:47:14Z

Based on #332

bekos · 2013-04-26T13:58:06Z

I need a little help with the mousewheel tests. Is there any example or reference how to test this scroll functionality for the time inputs?

jhiemer · 2013-04-28T06:53:01Z

Do you have a plnkr for the timepicker directive anywhere?

bekos · 2013-04-28T10:52:10Z

It is not the final implementation as may things have changed since I started writing this PR, but the overall behavior can be found on the initial discussion. #332

jhiemer · 2013-04-28T11:02:45Z

@bekos looks very nice as well. For this implementation I would only suggest a restriction of times, like suggested for the datepicker. Perhaps comparable to the filter solution I suggested.

bekos · 2013-04-28T13:21:12Z

@jhiemer Thank you. In this case I don't think it's good to pass the validation / restriction implementation into the directive. Someone can $watch the model (time) changes and validate it in order to provide some visual feedback if the selected time is wrong, instead of jumping times in the timepicker from 12 to 3, if 1 and 2 is restricted. I think this will look ugly. Again, some default $services for time comparison can later be added.

jhiemer · 2013-04-28T18:56:40Z

@bekos sounds like a plan. Great!

ajoslin · 2013-04-28T19:15:46Z

Hi @bekos, this looks great! 🎯

For the mousewheel, you'll have to mock it with jQuery (we use jQuery in the tests to make this kind of thing easier)

Try something like this:

function wheelThatMouse(delta) {
  var e = $.Event("mousewheel");
  e.wheelDelta = delta;
  return e;
}

it('should mousewheel', function() {
  myElm.trigger( wheelThatMouse(10) );
});

bekos · 2013-04-28T19:53:29Z

Hi again @ajoslin and thank you! :-D

Another thanks for your help on the mousewheel tests. I owe you! 👍

ajoslin · 2013-05-05T20:22:42Z

Alright, I have a few suggestions for improvement, mainly on the UI side.. I'm basing a lot of these thoughts on timepicker UIs like this that are standard already, like the android timepicker.

Improvements:

Let's use + and - instead of up and down, with button backgrounds on them.
AM/PM should be just one button with no + or -.
readonly should be default false on the input - I don't see why not, and it's the way these usually work. In fact, I don't see the use case for a readonly option at all.

Questions:

The number padding (eg 0 to 00) - won't that mess up user input? Perhaps just get rid of it? Or do you have an idea for a better solution?
How will we validate the inputs? eg so user can't type 99 or pizza. We don't want to use <input type="number"> because that makes our input box look ugly in this case.
How would the user setup restrictions on time at the moment? Eg if I only want time allowed between 3pm and 8pm.

Lastly, perhaps we should close this PR and re-open it with a branch on bootstrap main, so we can pull/push our little ideas to it. If that's ok with you?

bekos · 2013-05-06T09:17:27Z

@ajoslin I agree with the UI changes/improvements you propose, and these are changes that can be made just in the template. The initial template is the one that @ProLoser suggested and is used in some popular implementations of the timepicker for bootstrap. Example: http://jdewit.github.io/bootstrap-timepicker/

Probably we can provide two different templates, since templateUrl in 1.2 can be determined based on function, and use an attribute to select the template. What do you think of this approach, as a future plan?

I used default readonly as ng-keypress is not part of the stable library, to do some proper validation, but this can be easily changed if you want.
About the restrictions, you can see my opinion above.
Indeed zero padding is a problem for validating user input the way it works now. (watches every input change and pads the value). An alternative is to unpad on focus, allow pressing [0-9] and validate/pad on blur. Also, explicitly pad on model changes and buttons pressing. I don't agree to get rid of it, if we can handle it.

Feel free to close this one and create a new branch in main if you think this will also help others to participate to make things better :-)

pkozlowski-opensource · 2013-05-12T09:28:59Z

My 2 cents here: I don't think we should use any of the AngularJS 1.1.x features till it becomes stable. Many people are still on 1.0.x and we would exclude all of them.

@bekos @ajoslin where do we stand with this one? Are there any outstanding items to discuss / decide upon?
@bekos could you squash commits into one when all the items from the discussion are resolved?

bekos · 2013-05-12T10:03:12Z

@pkozlowski-opensource Totally agree that we should not use "unstable" features at this moment.

The matter here is that @ajoslin proposed a different template. In my opinion both templates deserve to be in the source files and offered to the users. So, the question (if you agree with me) is how do we offer these two alternatives to the users, at this moment?

The other thing is user's input validation. For the initial version I didn't want to focus on user's input validation, but to user's interaction with the provided controls. Actually, I was thinking for start to use span instead of input. So I default to readonly inputs. But @ajoslin is absolutely right that this is something that needs to be addressed.
Question again: do we merge this into the master as is (I feel that it is usable enough for an initial version) in order for others to contribute as well or do we port this into another branch and wait until it is feature complete?

bekos · 2013-05-17T00:26:05Z

OMG! I was doing this very wrong :-)

Refactored and dropped 40 LOC even if I added 30 LOC for user's input validation.

I finally changed template to a mix of what @ajoslin proposed & what was already there (changed meridian to button, but kept the up & down icons, instead of + & -). As I said before I added user's input validation, so the default state for readonly is false. Padding is still there. Time validation can be done by $watching model's value.

Has anyone time for a code review?

ajoslin · 2013-05-17T01:06:55Z

src/timepicker/timepicker.js

+        }
+      }, true);
+
+      var convert24to12Hours = function(hours) {


Couldn't this just be 'return hours % 12'?

No. The problem is with 0 and 12. Both must return 12 instead of 0 that mod returns.
Probably I can do this in one line :-)

ajoslin · 2013-05-17T10:49:27Z

I was interrupted by a phone call while reviewing this yesterday. I'll try to take a peak in a little bit.

bekos · 2013-05-17T11:02:24Z

@ajoslin Thank you. I don't like the way that user's input is validated. Probably you can come up with a better solution :-)

ajoslin · 2013-05-18T10:37:15Z

src/timepicker/timepicker.js

+  mousewheel: true
+})
+
+.directive('timepicker', ['$filter', '$parse', 'timepickerConfig', function ($filter, $parse, timepickerConfig) {


You could just inject padFilter here instead of $filter. Then you wouldn't have to call the filter('pad') getter here. Tiny unnoticeable performance boost 🎉

You are right. Thx.

ajoslin · 2013-05-18T10:45:25Z

Your validation looks fine to me :-) I like that you do it on blur. Messing with the ngModelCtrl $parsers pipeline of each hours/minute input would get funky IMO.

It looks a lot better now for sure :-)

bekos · 2013-05-19T14:07:52Z

After @ajoslin 's review & comments (thank you 👍 ) I think there is no other outstanding item at this moment.

ajoslin · 2013-05-22T16:37:45Z

It LGTM! Just pulled down the newest and tried it out.

I also love the little eyes you put on the docs for the observed attributes! 👀 😄

Anyone else have objections to merging this bad boy?

ajoslin · 2013-05-30T14:09:07Z

So I used it again .. and I dunno if I still agree with my earlier sentiment about validation :-)

I just used it and it really doesn't feel "angular-y" that the time only actually changes on blur.

It would be better to create sub-directives (hours-input, minutes-input) which hook into a timepicker controller and use ngModel $parsers to validate.

Also, I noticed that if you for example input "12:33" and the timestep is 15 minutes, it will force you to go back to "12:30". I think if the user inputs it should override the timestep, that makes the most since.

What do you think @bekos?

bekos · 2013-05-30T14:37:26Z

@ajoslin I am not so keen about the normalization of minutes. This is something I saw in a timepicker (I don't remember which one) and I tried to reproduce. Probably it will be better off. If user wants to normalize the initial minutes can do it in the controller probably.

About the validation what you describe is absolutely better, but it sounds like trouble :-)
Do you want me to give it a try?

bekos · 2013-05-30T14:43:47Z

Probably instead of extra directives, we can hook with more events, like keydown, blur and focus.

ajoslin · 2013-06-02T16:51:48Z

Yeah, that works.

bekos · 2013-06-03T13:19:19Z

@ajoslin Took even easier approach with ng-change in template and pad on blur.

I hope you like it now :-)

ajoslin · 2013-06-03T17:14:50Z

I like it! Good work.

I did find a bug though. To reproduce:

Make sure time is on 12 hours, then put an invalid hours amount in like 13. Date is invalid now
Switch to 24 hours
Time won't re-validate itself until you do an action.

It really does look good after that tiny bugfix though. It's really sleek now with the instant date change on input 👍

bekos · 2013-06-03T21:51:13Z

@ajoslin This was not a bug, it was more like a missing feature :-P Since the model was invalidated, the toggle of meridian, wasn't triggering a reset of values!

Anyway, I fixed it and added a test for Andy''s use case. I hope everything is(:question:) OK now!

ajoslin · 2013-06-04T00:03:08Z

Everything's OK now! (http://youtu.be/ocy_kp8HT_k)

LGTM, let's merge :-)

pkozlowski-opensource · 2013-06-09T11:23:20Z

@bekos could you merge it? Looks like it is fine now :-)

bekos · 2013-06-09T11:27:37Z

@pkozlowski-opensource Sure! I'll do it later today :-)

pkozlowski-opensource · 2013-06-09T12:57:14Z

Was playing with it so finally landed as 9bc5207. Yay!

bekos mentioned this pull request Apr 28, 2013

[Proposal] Datepicker directive #366

Closed

ajoslin reviewed May 17, 2013
View reviewed changes

ajoslin reviewed May 18, 2013
View reviewed changes

ajoslin mentioned this pull request May 22, 2013

timepicker #332

Closed

feat(timepicker): add timepicker directive

f2d0e7f

pkozlowski-opensource closed this Jun 9, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(timepicker): add timepicker directive #358

feat(timepicker): add timepicker directive #358

bekos commented Apr 26, 2013

bekos commented Apr 26, 2013

jhiemer commented Apr 28, 2013

bekos commented Apr 28, 2013

jhiemer commented Apr 28, 2013

bekos commented Apr 28, 2013

jhiemer commented Apr 28, 2013

ajoslin commented Apr 28, 2013

bekos commented Apr 28, 2013

ajoslin commented May 5, 2013

bekos commented May 6, 2013

pkozlowski-opensource commented May 12, 2013

bekos commented May 12, 2013

bekos commented May 17, 2013

ajoslin May 17, 2013

bekos May 17, 2013

ajoslin commented May 17, 2013

bekos commented May 17, 2013

ajoslin May 18, 2013

bekos May 18, 2013

ajoslin commented May 18, 2013

bekos commented May 19, 2013

ajoslin commented May 22, 2013

ajoslin commented May 30, 2013

bekos commented May 30, 2013

bekos commented May 30, 2013

ajoslin commented Jun 2, 2013

bekos commented Jun 3, 2013

ajoslin commented Jun 3, 2013

bekos commented Jun 3, 2013

ajoslin commented Jun 4, 2013

pkozlowski-opensource commented Jun 9, 2013

bekos commented Jun 9, 2013

pkozlowski-opensource commented Jun 9, 2013

feat(timepicker): add timepicker directive #358

feat(timepicker): add timepicker directive #358

Conversation

bekos commented Apr 26, 2013

bekos commented Apr 26, 2013

jhiemer commented Apr 28, 2013

bekos commented Apr 28, 2013

jhiemer commented Apr 28, 2013

bekos commented Apr 28, 2013

jhiemer commented Apr 28, 2013

ajoslin commented Apr 28, 2013

bekos commented Apr 28, 2013

ajoslin commented May 5, 2013

bekos commented May 6, 2013

pkozlowski-opensource commented May 12, 2013

bekos commented May 12, 2013

bekos commented May 17, 2013

ajoslin May 17, 2013

Choose a reason for hiding this comment

bekos May 17, 2013

Choose a reason for hiding this comment

ajoslin commented May 17, 2013

bekos commented May 17, 2013

ajoslin May 18, 2013

Choose a reason for hiding this comment

bekos May 18, 2013

Choose a reason for hiding this comment

ajoslin commented May 18, 2013

bekos commented May 19, 2013

ajoslin commented May 22, 2013

ajoslin commented May 30, 2013

bekos commented May 30, 2013

bekos commented May 30, 2013

ajoslin commented Jun 2, 2013

bekos commented Jun 3, 2013

ajoslin commented Jun 3, 2013

bekos commented Jun 3, 2013

ajoslin commented Jun 4, 2013

pkozlowski-opensource commented Jun 9, 2013

bekos commented Jun 9, 2013

pkozlowski-opensource commented Jun 9, 2013