Skip to content

Conversation

@timmywil
Copy link
Member

@timmywil timmywil commented Nov 6, 2015

Fixes gh-1712

src/css.js Outdated
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is been removed.

@timmywil
Copy link
Member Author

timmywil commented Nov 9, 2015

Added tests for percentages as well.

@mgol
Copy link
Member

mgol commented Nov 9, 2015

I like that the source diff is quite small. :)

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.

@timmywil timmywil force-pushed the width-1712 branch 3 times, most recently from 55b3f98 to 7afbbce Compare November 9, 2015 20:47
Copy link
Member

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" ).

Copy link
Member Author

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

screenshot 2015-11-09 16 00 18

Copy link
Member

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?

@timmywil timmywil closed this in 75b3cdd Nov 9, 2015
@timmywil timmywil deleted the width-1712 branch November 9, 2015 22:39
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

5 participants