Permalink
11 comments
on commit
sign in to comment.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Browse files
Fix #11469. Exclude margins from the negative property check.
- Loading branch information
Showing
1 changed file
with
6 additions
and
3 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
07c8a9b
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.
if ( prop.indexOf( "margin" ) ) {
looks a bit counter-intuitive when meant to "exclude margin". I know it works because-1
(no matches) is consideredtrue
-ish in JavaScript, but > 0 is alsotrue
-ish. Perhaps useif ( prop.indexOf( "margin" ) === -1 ) {
instead?07c8a9b
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.
The condition only cares about
prop.indexOf( "margin" )
matching "margin" at0
: [margin]Top, [margin]Left, [margin]Bottom, [margin]Right. So what is the point in the additional bytes?07c8a9b
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 dunno, code is not that clear to me either... !/^margin/.test( prop ) would have been better imo and happens to be a byte shorter.
07c8a9b
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.
@jaubourg I'm a sucker for a valid argument. +1
I'm still a little put off that this landed with no test to prove its failure.
07c8a9b
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.
@rwldrn I agree, this went in a little too easy. I keep up with jquery core as much as I can but I only found out about this fix via the release notes in the blog post..
07c8a9b
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.
Well, that's just me being lazy. I really wanted to get 1.7.2 out today but this was definitely a regression. I almost added
!== 0
but meh. I don't have time to test it but String.indexOf is probably faster than RegExp.test() if it makes any difference. We can make it pretty for 1.8 but if it hadn't have landed I suspect it would have been a strong case for 1.7.3.07c8a9b
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.
@dmethvin don't get me wrong, I support your decision to land this and for the same reason. I'm just not sure how a patch can even be written without a test accompanying it, know what I mean? Always write the test first and then fix the bug.
07c8a9b
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.
@dmethvin same as Rick. We all agree this needed to be in 1.7.2.
07c8a9b
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.
+1 to @louisremi with the fix, but also +1 to @rwldrn. IMHO, untested code is broken code. We're just as vulnerable to another regression here as we were before. I think I'll write the tests myself just to make sure I won't cause this regression in 1.8.
07c8a9b
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've already written this test and I'll send a PR today with additional tests.
Sorry for not doing it earlier.
Should I switch the condition to what @jaubourg proposed in the same PR or in a separate branch? I don't think the perf. difference matters, it's run only once, when the lib is loaded.
Sorry for the counter-intuitive mind ;-)
07c8a9b
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.
Thanks again @louisremi for the fix!