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

Isolated scroll #1155

Merged
merged 6 commits into from
Apr 25, 2013
Merged

Isolated scroll #1155

merged 6 commits into from
Apr 25, 2013

Conversation

kenearley
Copy link

@stof @wojcikstefan I've got it working in IE now. I would have just used that implementation on all browsers but it's pretty janky in FireFox

Thoughts?

This is a merge of PR #1122

cc: @pfiller

wojcikstefan and others added 4 commits April 4, 2013 18:51
… elasticsales-isolated-scroll

Conflicts:
	chosen/chosen.jquery.js
	chosen/chosen.jquery.min.js
	chosen/chosen.proto.js
	chosen/chosen.proto.min.js
@@ -48,6 +48,18 @@ class Chosen extends AbstractChosen
else
container_div.html '<a href="javascript:void(0)" class="chzn-single chzn-default" tabindex="-1"><span>' + @default_text + '</span><div><b></b></div></a><div class="chzn-drop" style="left:-9000px;"><div class="chzn-search"><input type="text" autocomplete="off" /></div><ul class="chzn-results"></ul></div>'

container_div.find('.chzn-results').bind 'mousewheel DOMMouseScroll', (evt) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this like this? We assign a variable a few lines later that cache's the results: @search_results

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: is there some reason event handlers aren't in register_observers?

@pfiller
Copy link
Contributor

pfiller commented Apr 19, 2013

I think the functionality is worthwhile, but this is not ready to merge as-is. The code is not consistent with the rest of Chosen's stye at all.

@kenearley kenearley mentioned this pull request Apr 19, 2013
There are differences in the way the Firefox and all other browsers
handle mousewheel events. There are two handlers to take care of this.
@kenearley
Copy link
Author

@pfiller It turns out that Firefox listens for the event DOMMouseScroll but then returns evt.type as mousewheel. This caused me to have to have separate event listeners for the two ways. Also, there are subtle differences between jquery and prototype. All in all, that makes for 4 relatively small methods. Two in each js library implementation. This is about as clean as it gets.

@pfiller
Copy link
Contributor

pfiller commented Apr 25, 2013

I didn't test in IE, but the implementation seems solid. I'm sold.

:shipit:

@pfiller pfiller closed this Apr 25, 2013
@pfiller pfiller reopened this Apr 25, 2013
Conflicts:
	chosen/chosen.jquery.min.js
	chosen/chosen.proto.min.js
	coffee/chosen.jquery.coffee
kenearley pushed a commit that referenced this pull request Apr 25, 2013
@kenearley kenearley merged commit 1760a47 into master Apr 25, 2013
@kenearley kenearley deleted the elasticsales-isolated-scroll branch April 25, 2013 20:30
@kenearley kenearley mentioned this pull request May 1, 2013
pfiller added a commit that referenced this pull request May 2, 2013
Fixes bug with Isolated Scrolling #1186
Remove unused get_side_border_padding #1169
Fix issue when using both jQuery & Prototype #1168
Fix choices_click method #1163
Isolate Chosen Scrolling #1155
Fix Right-to-Left scrollbar issue #1159
Add support for labels #1152
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants