feat(timepicker): added ability to clear timepicker control #5299
Conversation
asurinov
changed the title
|
@Foxandxss what is your thoughts? I'm a little wary since this is a breaking change with UX ramifications. Perhaps this should be hidden behind a config flag to prevent it from becoming a breaking change. |
| @@ -464,6 +474,12 @@ angular.module('ui.bootstrap.timepicker', []) | |||
| return newDate; | |||
| } | |||
|
|
|||
| function modelIsEmpty(){ | |||
| @@ -165,7 +165,7 @@ angular.module('ui.bootstrap.timepicker', []) | |||
| var hours = +$scope.hours; | |||
| var valid = $scope.showMeridian ? hours > 0 && hours < 13 : | |||
| hours >= 0 && hours < 24; | |||
| if (!valid) { | |||
| if (!valid || $scope.hours === '') { | |||
There was a problem hiding this comment.
Just a tip, the empty string is falsey so this and all your other checks of this nature can be simplified.
There was a problem hiding this comment.
This is fine - valid is a boolean, as can be seen from the two lines prior. I did do a double take too, but it checks out.
|
On one hand, I'm ok with this being a BC behind an optional config flag. On the other, we talk about API bloat. I'm on mobile right now so I can't look closely enough, but what if we let the user optionally specify their own validation function so it's totally up to them? |
|
I think it's a normal behavior of any control to have an empty state and this state must be valid |
|
@asurinov, while I tend to agree with that statement, we still need to balance feature vs. usability. there is such a thing as too much control and if a component gets to that point, it becomes unwieldy and confusing and actually creates more problems than it solves. See the datepicker for what I mean. |
|
After letting this sit a little while, I'm ok with this breaking change. This PR LGTM - the only question left now is when should we schedule this change - 1.2.3 or 1.3.0? |
|
So when will it be merged? |
|
@asurinov, please remember we are all unpaid volunteers. while i understand and can appreciate some level of impatience, please be a little more discerning with your word choice. we'll get this merged. |
|
There was some objection by @Foxandxss on this PR, as to this particular feature, which is the primary reason for holding up this being merged. |
|
I just don't like breaking changes for such feature, but also don't bloat the API. So I am not entirely sure but I don't mind it either. |
|
Alright, then I'll merge it - one could write a directive with an appropriate $parser/$formatter if one wants the previous behavior. |
Closes #5293