Skip to content
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

Fix animateParentHeight height calc when border-box #504

Conversation

franciscolourenco
Copy link
Contributor

You usually don't merge pull requests, but this was reported some time ago,
and I needed a quick fix for myself, so why not upload it. Even though
animateParentHeight is probably going to be overhauled.

animateParentHeight was over-calculating the height of the children elements
which have the box-sizing set to box border. This resulted in the parent
being animated too tall: http://jsbin.com/xakipigino/1/edit

For elements with box-sizing: "border-box", the height value returned by
Velocity.CSS.getPropertyValue(element, 'height') already includes
padding and border. So in these cases, padding and border should no be
added when calculating the total height occupied the element:
http://jsbin.com/wiyejuxequ/1/edit

animateParentHeight was over-calculating the height of the children elements
which have the box-sizing set to box border. This resulted in the parent
being animated too tall: http://jsbin.com/xakipigino/1/edit

For elements with box-sizing: "border-box", the height value returned by
Velocity.CSS.getPropertyValue(element, 'height') already includes
padding and border. So in these cases, padding and border should no be
added when calculating the total height occupied the element.
@franciscolourenco franciscolourenco changed the title Fix animateParentHeight height calc when box-border Fix animateParentHeight height calc when border-box May 2, 2015
@Rycochet
Copy link
Collaborator

Rycochet commented May 4, 2015

Just as a pure performance issue - but given all the possible values for border-box, you could just compare the first character with the letter "b" instead. Not quite as readable, but gives the same result ;-)

@julianshapiro
Copy link
Owner

Will check this out this weekend

@anttis
Copy link

anttis commented Oct 18, 2015

Any update available on this issue?

@Rycochet Rycochet merged commit cb1cd36 into julianshapiro:master Aug 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants