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

trigger "chosen:no_result" event #1712

Merged
merged 1 commit into from
Feb 7, 2014
Merged

Conversation

lightxu
Copy link

@lightxu lightxu commented Dec 31, 2013

Trigger "chosen:no_result" event if function no_results is called.

A feature required by me. XD

@stof
Copy link
Collaborator

stof commented Jan 1, 2014

this should also be implemented in the Prototype version.

and the Chosen instance should be available in the event to be consistent with other events triggered by the plugin

@Cynaraa
Copy link

Cynaraa commented Jan 6, 2014

I agree with the others. This event is necessary since we will need to provide other options in case there are no results

@lightxu
Copy link
Author

lightxu commented Jan 6, 2014

I've sent another patch, see #1717.
Since I'm not familiar with prototype, I hope someone could help me test it. :)

@tjschuck
Copy link
Member

tjschuck commented Jan 6, 2014

Hi @lightxu,

Can you please do both in the one commit here? On this same branch from your repo (patch-1), please amend this commit with your changes (make your changes, add the file, and git commit --amend). Then you should have one commit with both changes -- force push up with git push -f and this pull request will be updated to have both changes.

Thanks for your patch! I'm going to close #1717 for you in favor of keeping this one.

@tjschuck
Copy link
Member

tjschuck commented Jan 6, 2014

@lightxu Ah, I also just saw that this is multiple commits. You can instead just make the commit normally and then squash all three down into just the one logical commit. Instructions here.

@lightxu
Copy link
Author

lightxu commented Jan 6, 2014

Hi @tjschuck,
Thank you very much for instructions. I was changing the file directly through web interface, which is not so convenient...Any way, I've put changes on both files into one commit.
Looking forward to see this feature in chosen!

@tjschuck
Copy link
Member

tjschuck commented Jan 6, 2014

Thanks, @lightxu!

I mostly just triage the issues and pull requests here, so you'll have to wait for one of the @harvesthq/chosen-developers to actually review the code and merge it. Hopefully they'll sweep through soon and give you any feedback if necessary.

@pfiller
Copy link
Contributor

pfiller commented Jan 7, 2014

Thanks @lightxu

I have one nitpick and then I think this is good to go. The method that calls this is no_results, not no_result. I think we should be consistent and make the event match:

chosen:no_results

We should also update the documentation to include this new event.

@@ -406,6 +406,7 @@ class Chosen extends AbstractChosen
no_results_html.find("span").first().html(terms)

@search_results.append no_results_html
@form_field_jq.trigger("chosen:no_result")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably should add {chosen: this} as second argument like all other triggers.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As @stof mentioned before:

and the Chosen instance should be available in the event to be consistent with other events triggered by the plugin

@pfiller
Copy link
Contributor

pfiller commented Jan 7, 2014

Yes! And what @koenpunt said about providing a reference to Chosen.

@lightxu
Copy link
Author

lightxu commented Jan 7, 2014

Hi @pfiller @koenpunt

I've modified the code according to your comments. Although I'm not very clear about the chosen object. Since at first time, I modified in this way, but some error popup, which makes me think it is a bug.

I tested the jquery version and it works, I didn't test prototype version.

Thank you all for guiding me through this.

@@ -400,6 +400,7 @@ class @Chosen extends AbstractChosen

no_results: (terms) ->
@search_results.insert @no_results_temp.evaluate( terms: terms )
@form_field.fire("chosen:no_result", {chosen: this})
Copy link
Member

Choose a reason for hiding this comment

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

no_results here as well (instead of no_result).

pfiller added a commit that referenced this pull request Feb 7, 2014
Empty search triggers "chosen:no_results" event
@pfiller pfiller merged commit c62d160 into harvesthq:master Feb 7, 2014
@pfiller
Copy link
Contributor

pfiller commented Feb 7, 2014

Thanks so much, @lightxu

@lightxu lightxu deleted the patch-1 branch February 7, 2014 04:04
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.

6 participants