Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

feat(rating): Added rounding logic to rating value #3415

Closed
wants to merge 6 commits into from
Closed

feat(rating): Added rounding logic to rating value #3415

wants to merge 6 commits into from

Conversation

moderndegree
Copy link

I added logic that rounds off decimals to display a more accurate number of icons (i.e. stars).
This should close out issue #3413.

@karianna karianna added this to the 0.13.0 milestone Mar 21, 2015
@@ -58,6 +58,12 @@ angular.module('ui.bootstrap.rating', [])
this.render = function() {
$scope.value = ngModelCtrl.$viewValue;
};

$scope.$watch('value', function(value) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take a look

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@moderndegree
Copy link
Author

Apparently, angular.isNumber uses typeof so I am using it. Guess I shouldn't make such assumptions in the future.

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 % 1 === -0. Though value should never be negative..

How about doing integer truncation so it'll be value << 0 === value instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't it be value << 0 !== value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, sorry for that typo.

@chrisirhc
Copy link
Contributor

@moderndegree Actually, the change to use angular.isNumber has more to do with the enhancing the readability of the code. Using angular.isNumber also avoids the use of a magical constant 'number' which can be confusing to someone new to JavaScript.

@moderndegree
Copy link
Author

@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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant for this to be the title of the new test ^ instead of changing the name of an old test.

@chrisirhc
Copy link
Contributor

Just saw something else, didn't see that earlier. Otherwise, looks good.

@chrisirhc chrisirhc self-assigned this Mar 23, 2015
@moderndegree
Copy link
Author

@chrisirhc Good catch. I blame the scotch.

@chrisirhc
Copy link
Contributor

Thank you! I've squashed the commits and cleaned up some whitespace.
Take note next time to clean up that nasty trailing whitespace!

ayakovlevgh pushed a commit to ayakovlevgh/bootstrap that referenced this pull request Mar 24, 2015
- 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants