Skip to content

Position: Added better collision detection for flip and fit, added visual tests for each and updated the unit tests to take the changes into account. #418

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 3 commits into from

Conversation

kborchers
Copy link
Member

In the process, I removed the rounding that was being done to the position since older jQuery couldn't handle the fractions. There was another pull for this same issue and I have merged the unit tests from that pull into this commit.

@jzaefferer
Copy link
Member

Running the regular position unit tests with this applied, I get a bunch of failing tests. Most similar to the first one here: http://bassistance.de/i/319d21.png
Same results in Chrome 12, Firefox 5 and Safari 5.

@kborchers
Copy link
Member Author

Yeah, I wasn't sure if it was just me not understanding the unit tests or not. I'll look at those a little later on today and see what I can figure out. Something I noticed yesterday though was now it seems like some browsers round fractional positions and others don't so I'm not sure how I should handle that if they are slightly different.

@inukshuk
Copy link

inukshuk commented Aug 4, 2011

kborchers, I think the best approach for master is to never manipulate position in a way that could truncate pixel values (i.e, do not use Math.round, parseInt) and be extra careful when using division or subtraction. The thing is, if the browser returns fractions you want to keep them.

For 1.8-stable which may be distributed with older versions of jQuery you can test for support.fractions (defined in the position closure in this pull request: if false, the consensus was to use Math.round which may still lead to off-by-one errors in rare cases but at least gives you stable results.

@kborchers
Copy link
Member Author

@jzaefferer This should be good to go now. All tests pass in Chrome, Firefox and Safari and all tests but the 5280 bug test pass in IE which I believe is a rounding thing. Not sure what to do about that other than @inukshuk's method for checking support of fractions.

@jzaefferer
Copy link
Member

This breaks the flipping in the regular position demo. Set position to right (already the default in position_within), then drag to the right. It doesn't flip, instead just moves out of the screen (or the scrollable container).

…sual tests for each and updated the unit tests to take the changes into account. In the process, I removed the rounding that was being done to the position since older jQuery couldn't handle the fractions. There was another pull for this same issue and I have merged the unit tests from that pull into this commit.
@kborchers
Copy link
Member Author

OK, I think that does it. Let me know if you find anything else. The one bug test fails in IE because IE rounds fractional values but otherwise all tests pass.

@kborchers
Copy link
Member Author

I am going to close this pull request and submit another when I have this working better. I have noticed some oddities in fit tonight so I need to keep working.

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