feat(rating): Added rounding logic to rating value #3415
Conversation
| @@ -58,6 +58,12 @@ angular.module('ui.bootstrap.rating', []) | |||
| this.render = function() { | |||
| $scope.value = ngModelCtrl.$viewValue; | |||
| }; | |||
|
|
|||
| $scope.$watch('value', function(value) { | |||
There was a problem hiding this comment.
This seems to be an unnecessary & unperformant approach.
A better approach would be to create a formatter, that way it only gets run when the model value changes.
| @@ -12,6 +12,13 @@ angular.module('ui.bootstrap.rating', []) | |||
| this.init = function(ngModelCtrl_) { | |||
| ngModelCtrl = ngModelCtrl_; | |||
| ngModelCtrl.$render = this.render; | |||
|
|
|||
| ngModelCtrl.$formatters.push(function(value) { | |||
| if (typeof value !== 'undefined' && value % 1 !== 0) { | |||
There was a problem hiding this comment.
Is there any reason why we can't just do
if (value && angular.isNumber(value)) {
here? It seems you are trying to test for whether it is a number, this would be a better check.
|
Apparently, |
| @@ -12,6 +12,13 @@ angular.module('ui.bootstrap.rating', []) | |||
| this.init = function(ngModelCtrl_) { | |||
| ngModelCtrl = ngModelCtrl_; | |||
| ngModelCtrl.$render = this.render; | |||
|
|
|||
| ngModelCtrl.$formatters.push(function(value) { | |||
| if (angular.isNumber(value) && value % 1 !== 0) { | |||
There was a problem hiding this comment.
-1 % 1 === -0. Though value should never be negative..
How about doing integer truncation so it'll be value << 0 === value instead?
There was a problem hiding this comment.
wouldn't it be value << 0 !== value?
There was a problem hiding this comment.
Indeed, sorry for that typo.
|
@moderndegree Actually, the change to use |
|
@chrisirhc Makes sense. I've also swapped out the modulo for bitwise logic. |
| expect(element.attr('aria-valuenow')).toBe('3'); | ||
| }); | ||
|
|
||
| it('rounds off the number of stars shown with decimal values', function() { |
There was a problem hiding this comment.
I think you meant for this to be the title of the new test ^ instead of changing the name of an old test.
|
Just saw something else, didn't see that earlier. Otherwise, looks good. |
|
@chrisirhc Good catch. I blame the scotch. |
|
Thank you! I've squashed the commits and cleaned up some whitespace. |
- Moved rounding logic into a formatter - Checking for number instead of checking if undefined - Using angular.isNumber for rounding logic - Using bitwise instead of modulo to check for decimel Fixes angular-ui#3413 Closes angular-ui#3415
I added logic that rounds off decimals to display a more accurate number of icons (i.e. stars).
This should close out issue #3413.