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

Calculate margins #13

Closed
souporserious opened this issue Nov 9, 2015 · 6 comments
Closed

Calculate margins #13

souporserious opened this issue Nov 9, 2015 · 6 comments
Labels

Comments

@souporserious
Copy link

Margins that spill out of the container are not taken into account in the height calculation. I know Velocity handles this by just animating the margin, which can be done, but wanted to see how you feel about this.

@nkbt
Copy link
Owner

nkbt commented Nov 9, 2015

I prefer to add this into Gotcha section of readme rather then calculate them myself. That would decrease speed. though can be quite easily solved by wrapping either children or Collapse itself.

I can be wrong though, just did not find the case where margins cannot be avoided using Collapse.

@souporserious
Copy link
Author

Yeah it can be worked around with adding padding to the container, or anything else that gets rid of collapsing margins. Here's an example of how it gets dorked http://codepen.io/souporserious/pen/a060aeb382b647b32deb73346ac9de76/

@nkbt
Copy link
Owner

nkbt commented Nov 9, 2015

The problem with margins is that they are "collapsing". Having two elements stick to each other vertically - their margins will overlay. And that works not all the time. They also "leak" outside of elements (in your example H1 margins leak outside of wrapper div). There are some hacks to fix that in place, but I'd avoid trying to fix them "in general". Rather then "polyfilling" conceptually broken css, I'd prefer to add some usage restrictions. Especially considering they are not that ridiculous, just cutting off some edge cases.

@nkbt
Copy link
Owner

nkbt commented Nov 9, 2015

Look at this tiny fix: http://codepen.io/nkbt/pen/avRXPZ?editors=001

Adding overflow: hidden to wrapper div, "fixes" it. Like.... Really. That works. But it is an edge case, not a generic solution =(.

This is why I'd prefer to add a section to the README with these cases, examples and possible solutions. I believe it would be better approach.

@nkbt nkbt added the wontfix label Dec 20, 2015
@nkbt nkbt closed this as completed Dec 20, 2015
@ingro
Copy link

ingro commented Jan 6, 2016

Sorry for commenting on this, but adding a div with overflow:hidden works for the margin issue but doesn't work well with the keepCollapsedContent option. It seems like only the wrapper div is kept, while the children inside are not and throws various errors while interacting with them.

@nkbt
Copy link
Owner

nkbt commented Jan 6, 2016 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants