search

ehynds / jquery-ui-multiselect-widget

jQuery UI MultiSelect widget
GNU General Public License v2.0
1.37k stars 692 forks source link

ISSUE 837: Use jquery offset instead of .position(). #838

Closed rasben closed 5 years ago

rasben commented 5 years ago

What changes are you proposing? Why are they needed?

As i describe in my issue, my menu gets placed in a strange random position. I found out that the issue is with the jQuery .position() function, which for some reason gets confused on my setup. Both the left and top properties are wrong - if I base it on the .offset() functionality, then it works fine

I have a feeling that this fix can have some pretty crazy consequences - and it really surprises me that noone else seems to have this issue..

Screenshot 2019-03-11 at 16 18 19

Related Issue numbers

https://github.com/ehynds/jquery-ui-multiselect-widget/issues/837

Pull Request Approval Checklist:

rasben commented 5 years ago

Pull Request Approval Checklist: Tests Ran and 0 Failing Tests Added/Updated Impacted Demos Updated Impacted i18n Code Updated

Is this something I need to do, or is it the maintainers job? If I need to do it, how do I run these tests? :)

mlh758 commented 5 years ago

You should base your changes off of the version 3 branch, I'm trying to keep all future changes there (and I should really just merge it at some point).

Using the version3 branch doing the tests is pretty easy. The package.json has has a script in place so just npm install and npm run test.

I'm going to look through the commit history though, I have some memory of explicitly moving away from offset to use position due to some other error.

mlh758 commented 5 years ago

Never mind on the position thing, that was somewhere else. I think this should be good to merge once you've verified the tests and made the changes to version3 instead of master. The code should be pretty similar.

Thank you for contributing.

You can also try your changes against the demo page by running bundle exec jekyll serve in the docs folder. You'll need ruby and bundler installed for that though. I'm trying your changes out there now.

rasben commented 5 years ago

Ok great - I'll do those things and get back to you, probably today, maybe tomorrow

Thanks!

rasben commented 5 years ago

Ok Ive updated/commited/pushed and changed against V3 @mlh758 :)

I ran the tests and they didnt cause any problems:

Took 821 ms to run 334 tests. 334 passed, 0 failed.

Ruby is a bit fucked up on my machine right now, so I cant test the demo site sadly - could you perhaps check? :)

As mentioned before (and i guess as you know), this is a change that works fine for me, but I am a little worried that it would break lots of scenarios out there for people, for god knows what reasons - Im not even sure why the original is an issue on my setup after all..

I guess seeing as version3 is still in pre-release, its at their own peril to update though :)

mlh758 commented 5 years ago

I finally got around to testing this in a modal and it looks good.