-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix(core): drop the toBoolean function #7960
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,16 @@ describe('Binder', function() { | |
$rootScope.hidden = 'false'; | ||
$rootScope.$apply(); | ||
|
||
assertHidden(element); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh wait, misread the above line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a nit, but I would personally prefer it if each of these separate test cases within this function were in their own tests --- see what Igor thinks I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, the whole file is written in this way, though. I guess we can improve it incrementally while we touch it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...that said, tests here are quite simple, an error will tell you where the problem lies anyway and wrapping it in separate its would add boilerplate for @IgorMinar, what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is one of the oldest test files. we are slowly killing these tests and moving them to other more appropriate files. so don't make big refactorings here. |
||
|
||
$rootScope.hidden = 0; | ||
$rootScope.$apply(); | ||
|
||
assertVisible(element); | ||
|
||
$rootScope.hidden = false; | ||
$rootScope.$apply(); | ||
|
||
assertVisible(element); | ||
|
||
$rootScope.hidden = ''; | ||
|
@@ -267,6 +277,16 @@ describe('Binder', function() { | |
$rootScope.show = 'false'; | ||
$rootScope.$apply(); | ||
|
||
assertVisible(element); | ||
|
||
$rootScope.show = false; | ||
$rootScope.$apply(); | ||
|
||
assertHidden(element); | ||
|
||
$rootScope.show = false; | ||
$rootScope.$apply(); | ||
|
||
assertHidden(element); | ||
|
||
$rootScope.show = ''; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just
if (attr.ngTrim !== 'false') {
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This catches the case where
attr.ngTrim === false
as well asattr.ngTrim === 'false'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not clear that we really want to test for it to equal 'false' if the point of this PR is to get rid of toBoolean-like behaviour, though. Why not just
!attr.ngTrim
? I guess we aren't parsing it, hmm.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry for the noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it catches both when
attr.ngTrim === undefined
(notfalse
) and whenattr.ngTrim === 'false'
.attr.ngTrim
is either undefined or a string, like allattr
values.So yeah, we're not parsing it (I don't know why), so we have to sort-of parse it here.
Btw, that also means
ng-trim="some_falsy_variable"
won't work, why is it implemented in this way?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cost of watching, probably
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we just assumed that this is a "static" setting so we didn't want to complicate the implementation with parsing and full expression support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should consider moving this to ngModelOptions now that we have it. but that's for a separate PR.
@petebacondarwin what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree @IgorMinar - it is a good candidate for ngModelOptions. @shahata - perhaps another one of your nice PRs?