-
Notifications
You must be signed in to change notification settings - Fork 20.5k
Dimensions: properly manipulate non-px values #2695
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/css.js
Outdated
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 will force a synchronous layout, right? I suppose there's no way to avoid it but I wonder if there is a less expensive way to do it, for example with a different element attached to the body.
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 was thinking that using an element added to the DOM would trigger layout even more.
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 is been removed.
|
Added tests for percentages as well. |
|
I like that the source diff is quite small. :) |
test/unit/dimensions.js
Outdated
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.
Eeek! Pick a different parent so we're not screwing with the fixture container.
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.
Heh, make up your mind. I don't see the danger if the CSS is reset, but I'll change it.
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 said the font-size should be explicit (which I stand behind), but we should never be manipulating #qunit-fixture directly—only its contents are fair game.
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.
Yea, thought you were suggesting to use #qunit-fixture and be explicit about font-size when you asked why I was appending to #nothiddendiv, but I think I misread that.
55b3f98 to
7afbbce
Compare
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.
That's not true for e.g. .css( "borderWidth", "+=2px" ).
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 think it is, actually.
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.
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.
/me slaps forehead
I guess that's kind of the point, huh?

Fixes gh-1712