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

chore(rating): remove redundant validation and add unit tests #3464

Closed
wants to merge 1 commit into from
Closed

chore(rating): remove redundant validation and add unit tests #3464

wants to merge 1 commit into from

Conversation

imagentleman
Copy link
Contributor

Changes:

  1. Unit tests for the onKeyDown method were missing (https://github.com/angular-ui/bootstrap/blob/master/src/rating/rating.js#L62) in order to get a 100% coverage.
  2. Angular's HTML compiler throws an error (https://code.angularjs.org/1.2.28/docs/error/$compile/ctreq) if the required controllers are not found (the rating directive has ngModel listed as the second one on https://github.com/angular-ui/bootstrap/blob/master/src/rating/rating.js#L73). The error prevents the link method from being executed (i.e ngModel is not provided on the HTML) and the ngModelCtrl variable can never be falsy.

Working Plunker with the changes (line 3122 of ui-bootstrap.js shows the validation removed):
http://plnkr.co/edit/68xfeULwQhaHeq3W5mD1?p=preview

Plunker with the current version of the rating directive:
http://plnkr.co/edit/XBR4r3OuDSknWwgvpjPE?p=preview

Both behave the same (with and without the validation).

@pkozlowski-opensource
Copy link
Member

The quality PRs like this make me smile. Thnx @imagentleman !

@chrisirhc chrisirhc added this to the 0.13.0 milestone Mar 29, 2015
@imagentleman
Copy link
Contributor Author

Wow, thanks Pawel!

@realityking
Copy link
Contributor

Alternatively, ngModelController could be made optional. In the past I've found that less surprising for developers.

@wesleycho wesleycho self-assigned this Mar 29, 2015
@wesleycho wesleycho closed this in 31c2694 Mar 29, 2015
@wesleycho
Copy link
Contributor

Thanks for this @imagentleman - as @pkozlowski-opensource said, this is good work.

fernando-sendMail pushed a commit to fernando-sendMail/bootstrap that referenced this pull request Jul 9, 2015
fernando-sendMail pushed a commit to fernando-sendMail/bootstrap that referenced this pull request Jul 16, 2015
fernando-sendMail pushed a commit to fernando-sendMail/bootstrap that referenced this pull request Jul 16, 2015
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.

6 participants