Ticket 5645 - Position: Allow for arbitrary element to be containing element #254
Conversation
|
Thanks, can you please add unit tests for this before we start reviewing it? |
|
Would also help to modify one of the demos or visual tests, e.g. demos/position/default.html to show off the new behaviour. Once this feature landed, we just need to make the collision detection itself smarter (#5284) and we've got the perfect positioning utility. |
|
Just came across a issue with the collision detection. If the |
|
The unit tests are coming, I promise. They are a little harder to implement than I thought; I'm not very familiar with qunit. Should I just duplicate all the existing tests and make them work for 'within'? |
|
yeah, duplicating the existing collision tests and changing within to some other element should be fine. If there turn out to be any details of the tests that need to change, just let us know so we can figure out if we need to make decisions about the implementation (like the border question you brought up). |
|
Okay, unit test have been added. Some of them I don't think were relevant, such as any test where "of" is not a child of "within", so those are commented out. Also, one test fails. I'm not sure what is going there so I need someone else to take a look at the test for me, if you don't mind. It is test #24 "position: within: collision: fit, window scrolled" (window here is technically the "within" element but I kept the test names the same so it was easy to find the original test if needed). |
…e the results appear on top of the qunit-fixture
|
Thanks, I'll take a look at this tomorrow. |
| var win = $( window ), | ||
| overLeft = win.scrollLeft() - data.collisionPosition.left, | ||
| overRight = data.collisionPosition.left + data.collisionWidth - win.width() - win.scrollLeft(); | ||
| var win = data.within, |
There was a problem hiding this comment.
let's change this variable name to within to avoid confusion
| @@ -315,7 +314,7 @@ test( "collision: fit, with offset", function() { | |||
| test( "collision: fit, window scrolled", function() { | |||
| if ( scrollTopSupport() ) { | |||
| var win = $( window ); | |||
| win.scrollTop( 300 ).scrollLeft( 200 ); | |||
| $( window ).scrollTop( 300 ).scrollLeft( 200 ); | |||
There was a problem hiding this comment.
Hu? What's wrong with the win variable?
| outerHeight = isWindow ? win.height() : within.outerHeight(), | ||
| overTop = withinOffset - data.collisionPosition.top, | ||
| overBottom = data.collisionPosition.top + data.collisionHeight - outerHeight - withinOffset, | ||
| newTop; |
|
Very much looking forward to merging this - should make fixing this demo very easy: http://view.jqueryui.com/master/demos/index.html#menubar|default |
…eneficial, and making some other changes based on the code review
|
Hey guys, just wondering if you need anything else in order to get this pull in. Let me know! |
|
Nothing at this time, we've just been really busy with other work. We'll be reviewing this very soon. |
|
Ran the unit tests in IE6, 7, 8 and 9, a bunch of within tests fail in all of them. Could you check those? |
|
Just ran them in IE 6, 7, 8, and 9 and passed all of them. I believe some of the tests' expected values require that the window be scrolled all the way to the top and left ( e.g. scrollTop(0).scrollLeft(0) ). This was the default functionality of the tests and was not caused my my code modifications. Try running them again with a maximized browser window and make sure you are scrolled top and left. If you still find issues let me know which tests failed and your browser environment (os, browser version, screen size) and I'll see what I can do. |
|
I ran in a VM with 1024x768 resolution, all windows maximised. Here's part of the IE7 result: http://bassistance.de/i/a8398f.png last one: http://bassistance.de/i/b9770f.png Reran just now and can't reproduce :/ I'll do some more testing, if it looks good, I'll merge. |
|
Checked the code once more, looks very solid. Which doesn't mean much, considering the complexity of the collision code :/ Anyway, one issue I noticed, which I'd like to see resolved: While testing with the new visual test page, it becomes obvious the scrollbars aren't taken into account. The collision only happens on the actual border of the element, so it gets overlayed by the scrollbar before hitting that border. This works fine when collisioning with the window. |
|
Actually, could you (also) merge latest master to your branch? Just now I tested your branch, without merging to master, that was the difference. |
|
Yes to both comments above. :-) Do any of the UI plugins or core already have a method for determining scrollbar "offset" already? I'm pretty sure I saw a recent pull request that needed to do this (calculate scroll bar width/height) and they just did it within the plugin itself (sorry, I really can't remember what the pull request was for or who the author was). If a solid method for scrollbarOffset doesn't already exist should it be added to core so it can be re-used by the other plugins? And if so, should I open another ticket for this feature so it can be tracked? |
|
I don't think there's anything like that, but if there is it would be hidden inside one of the interaction plugins. I'm pretty sure that @brandonaaron had scrollbar code in the old dimensions plugin, but I think he dropped it at some point. |
|
How about putting those helper functions onto $.position, that way we can at least reuse them within the tests? Maintaining them in two places is not a good idea. Tested in Chrome, IE 6 and 9, unit tests pass, visual test looks good in Chrome and IE9, in IE6 I don't get the horizontal scrollbar on the within-container, and draggable doesn't scroll the window to the right, so can't even test with the vertical scrollbar. |
…scrollbar properties to $.position
|
Just noticed the commit you added. Too bad notifications aren't sent for additional commits. The commit itself looks good, will do some more testing. |
|
It live! Thanks a lot for the contribution, sorry it didn't make it within the rewardjs timeframe. If you're still interested, there are a few more open position issues (http://bugs.jqueryui.com/report/10?P=position), especially the Smart Collision Detection would be a great addition: http://bugs.jqueryui.com/ticket/5284 Come by on Freenode in #jqueryui-dev or ping me on Skype (joern.zaefferer) if you want to discuss that or other options. |
|
Its good to see it go live...and that there weren't any crazy merge conflicts. No problem. I won $200 bucks for some other bug. I'm pretty busy at the moment, when things clear up a bit I'll see if I can do some damage to some of those other issues. |
Position: Allow for arbitrary element to be containing element.
Fixed #5645 http://bugs.jqueryui.com/ticket/5645