Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Make choices_click() method more readable. #1163

Merged
merged 3 commits into from
Apr 25, 2013

Conversation

kenearley
Copy link

@pfiller

This is in response to issue #193. I believe I have the logic rewritten to be cleaner. However, I'm not sure that the behavior is quite right. Clicking any of the already chosen items in the input field will open the list except when the field has focus. Is that correct behavior?

@kenearley
Copy link
Author

@stof Your input is appreciated, as well.

@pfiller
Copy link
Contributor

pfiller commented Apr 25, 2013

After testing this, I don't think the choices_click method has ever worked right. This event bubbles properly to container mousedown (which then shows the results if need-be). I think we can kill the handler and this function in a separate PR and close this.

@kenearley
Copy link
Author

Ok. Closing this.

@kenearley kenearley closed this Apr 25, 2013
@kenearley
Copy link
Author

Looks like we do need this, for now.

@kenearley kenearley reopened this Apr 25, 2013
@pfiller
Copy link
Contributor

pfiller commented Apr 25, 2013

I guess we need this to open the search results when a field is active. Fair enough.

Much easier to read and seems to work the same as current Chosen. I think the initial point was to not open search results on choice click, but that's not what the actual result of the method was. So I say carry on.

:shipit:

Conflicts:
	chosen/chosen.jquery.min.js
	chosen/chosen.proto.min.js
kenearley pushed a commit that referenced this pull request Apr 25, 2013
Make choices_click() method more readable.
@kenearley kenearley merged commit 69030a5 into master Apr 25, 2013
@kenearley kenearley deleted the clean-up-choices-click-method branch April 25, 2013 21:08
choices_click: (evt) ->
evt.preventDefault()
if( @active_field and not($(evt.target).hasClass "search-choice" or $(evt.target).parents('.search-choice').first) and not @results_showing )
this.results_show()
this.results_show() unless @results_showing
Copy link
Collaborator

Choose a reason for hiding this comment

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

as it is now the same in both versions, it could be moved to AbstractChosen

Copy link
Author

Choose a reason for hiding this comment

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

Cool. I'll do that.

Copy link
Author

Choose a reason for hiding this comment

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

This was a small enough change. I committed straight to master: 9969a46

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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants