Permalink
Show file tree
Hide file tree
13 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 #10877. Make outerWidth/Height a setter. Closes gh-783.
- Loading branch information
1 parent
978acb9
commit e0151e5827d7091f311c82d9f951aaaa2688ba8c
Showing
4 changed files
with
130 additions
and
92 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
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
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
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
e0151e5
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.
Awesome. We can finally remove: https://github.com/jupiterjs/jquerypp/blob/master/dom/dimensions/dimensions.js. Great job!
e0151e5
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, is there anyway to detect this? We have a bunch of widgets that still work on older jQuery versions that depend on a setter. I'd like them to check if this is available before grabbing our plugin. Thanks!
e0151e5
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 test I added in jQuery UI is
$( "<a>" ).outerWidth( 1 ).jquery
. If it's chainable, then setters are implemented.e0151e5
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.
One more question, does this work with animation? It doesn't seem so. It can be very useful to animate outerWidth / innerWidth. We've done that here:
https://github.com/jupiterjs/jquerypp/blob/master/dom/dimensions/dimensions.js#L162
e0151e5
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.
@justinbmeyer, the fastest way to animate .outerWidth() is to set the elem to box-sizing:border-box, and then just animate .css('width')
e0151e5
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.
@scottgonzalez thanks!
@mikesherov thanks, but unfortunately that doesn't work in ie 6+7 and supporting that is still a reality for me as long as it is a reality for jQuery.
e0151e5
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.
@justinbmeyer: https://github.com/Schepp/box-sizing-polyfill
But also, you can use jQuery.css(elem, name, extra) for those animations. Of course, this is undocumented, but works for now. For example:
jQuery.fn.width(value)
=>jQuery.style(elem, "width", value, "content")
jQuery.fn.innerWidth(value)
=>jQuery.style(elem, "width", value, "padding")
jQuery.fn.outerWidth(value)
=>jQuery.style(elem, "width", value, "border")
jQuery.fn.width(value, true)
=>jQuery.style(elem, "width", value, "margin")
e0151e5
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.
You're talking about using jQuery.css inside my animate/step methods to set the value?
sets an element's width, but somehow accounts for padding?
e0151e5
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.
@justinbmeyer: Yes, and even corrects for whichever box-sizing you're using :-)
e0151e5
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.
e0151e5#L0R130 and e0151e5#L0R369
e0151e5
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.
Cool, but I'd still like to see jQuery able to easily animate these properties. We have a lot of widgets that accept any users styles, but animates outerWidth. We can't control their box model. Plus, if people are going to be using innerWidth outerWidth as setters, it's probably likely they will try using them as animate props.
If performance is a concern, maybe jQuery could look into bringing our "group styles" plugin:
https://github.com/jupiterjs/jquerypp/blob/master/dom/styles/styles.js
That can read multiple properties from getComputedStyle at once and has considerable (2x-3x) performance improvements in naive implementations of width / height.
e0151e5
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.
Yeah, I was going to enhance the performance of the augmentWidthorHeight function by doing exactly what you have there. Having to call getComputedStyle multiple times sucks. I need it to also grab the box-sizing as well, so it needs to be a bit less naive, and I have to worry about size for 1.8. It's coming though :-)
e0151e5
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.
@justinbmeyer, with regards to "easily" animate these properties, you can always use a propHook in effects.js:
jquery/src/effects.js
Line 345 in e0151e5