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

Added "Add New" support as requested in issue #5 #316

Closed
wants to merge 2 commits into from

Conversation

masonforest
Copy link

Added "Add New" support as requested in issue #5

@stof
Copy link
Collaborator

stof commented Oct 24, 2011

The callback should receive the value of the field

@@ -630,6 +634,10 @@
if (this.result_highlight) {
high = this.result_highlight;
high_id = high.attr("id");
if (high.hasClass("new-result")) {
this.options.new_item_callback();
Copy link
Author

Choose a reason for hiding this comment

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

Is this where you(@stof) would like to pass the value of the field? ie

this.options.new_item_callback(@search_field.val())

Couldn't you get that already in the callback with $(this).val()

Copy link
Collaborator

Choose a reason for hiding this comment

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

well, it would be easier IMO to pass the value.
Thus, this in the callback will not refer to the field with the way you call it currently AFAIS.

Choose a reason for hiding this comment

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

this refers to the settings object. Also, .val() returns the value of the <option> not it's content. How can I get the actual entered content?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean this inside the callback for the guy writing the callback. Using $(this).val() in the callback will not work as the this in the callback will refer to the root context (i.e. `window), not to the`` element

@stof
Copy link
Collaborator

stof commented Nov 29, 2011

You also need to implement it in the Prototype version.

And I still think that passing the value would be easier for the end-user. Let's see what @pfiller think about it.

@edrex
Copy link

edrex commented Dec 24, 2011

UI review:

Overall I like having an "Add new" option better than the approach taken in #326. It's more discoverable and more consistent.

However I would suggest these changes:

  • Only show Add new if
    1. the input field is non-empty
    2. and there isn't an option which is an exact match already
  • Change the text to "Add value" where value is the value currently in the input field

This would require updating the option whenever the input changes, but would improve usability IMO

@craue
Copy link
Contributor

craue commented Feb 3, 2012

Will this, #326, or any other approach to add a new value be included soon?

@dcalhoun
Copy link

+1 Would love to see the feature implemented. Any hopes of this happening in the near future?

@koenpunt
Copy link
Collaborator

@dcalhoun See my pull request (with docs) at #166 and see my branch at https://github.com/koenpunt/chosen. Just merged harvesthq/master back into it.



if high.hasClass "new-result"
@options.new_item_callback(evt)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the context for the callback should probably be changed to something more useful than this.options

@pfiller
Copy link
Contributor

pfiller commented May 26, 2012

Thanks @masonforest

I'm sorry it took so long to review this. There are several other pull requests that address adding new options and I wanted to give them all a fair shake. To my eyes, it looks as though #166 and #320 are more complete options so I'm going to focus on those. Feel free to weigh in on those threads.

@pfiller pfiller closed this May 26, 2012
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.

None yet

8 participants