Skip to content

effects: Fix for margin:auto(#4228); create wrapper with width:100% #285

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

Closed
wants to merge 1 commit into from

Conversation

tomykaira
Copy link
Contributor

effects: fix for css "margin: #px auto;".
Create wrapper with width:100%, when not floating. #4228 - margin: #px auto; not respected with effects.
"size" effect does not work in my environment, both the original and my version.

…t floating. #4228 - margin: #px auto; not respected with effects
@gnarf
Copy link
Member

gnarf commented May 18, 2011

Inspired approach to this problem.... Might actually tackle it.

Wouldn't a position other than static be a reason to not use width 100% as well?

How much testing have you done for this fix?

@tomykaira
Copy link
Contributor Author

https://gist.github.com/979147 is my test.
I think almost all margin definitions are tested.
And this passes all the existing unit tests.

@gnarf
Copy link
Member

gnarf commented May 18, 2011

Also, what about things that are display: inline-block or for that matter, any display other than block ?

@tomykaira
Copy link
Contributor Author

https://gist.github.com/980155
inline-blocks move as the original, with 2 exceptions.
Even if the target element is inline-block, its wrapper does not inherit "display", so the effect is wired. I think this is another bug about "display".

An exception: Slide goes to the other side of the window, not to the side of div. I think this is preferable.

The other exception: In blind effect with "direction: right", the target element jumps to the right side of the page.
This is not limited to "display: inline-block", but occurs with "block".
As far as I tried, it is impossible to fix this, because this effect needs two container for an element, one for wrapping with width:100%, the other for a shrinking and expanding animation.

If there is a way for building the second wrapper, no need for the wrapping one.
I think it is impossible, since there is no way for getting relative position information of margin:auto div.

How do you think about this?

@gnarf
Copy link
Member

gnarf commented May 21, 2011

Yeah --- Its not an easy problem to solve, I'll keep it in mind... I don't think this is ready for pulling yet though, its going to break stuff.

@tomykaira
Copy link
Contributor Author

I agree, if I have a new great idea, I will close this and commit again.

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

Successfully merging this pull request may close these issues.

3 participants