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

add key value support #16774

Closed
wants to merge 8 commits into from
Closed

Conversation

herregroen
Copy link

Adds support for key value pairs in the form of an array of pairs.

Key is added to the form through a hidden input.

@SkeLLLa
Copy link

SkeLLLa commented Oct 26, 2015

Done almost the same thing today. It would be nice to have such functionality.

@herregroen
Copy link
Author

Added a test for the new functionality.

If desired I can create similar tests for existing functionality.

@davycheung
Copy link

I see this hasn't been merged, any status on this one? Key value support would be splendid, thx.

@LeaVerou
Copy link
Owner

Yes, it would be splendid, but it’s also a pretty extensive change that I need to consider carefully. Extensive PRs take more time to be reviewed. Sorry!

@ricardobanegas
Copy link

Agree! Would be very good if key-value support was added

@ricardobanegas
Copy link

Any news on this PR? I've been using it for my project and works like a charm.

@herregroen
Copy link
Author

Just updated my pull request to the latest version since it seems I'm not the only one using it.

@LeaVerou
Copy link
Owner

@vlazar Any chance you could take a look at this? Don't merge, but I do need help reviewing as I can't find enough time to do it properly on my own. It seems to add a lot of code and I'm not sure it's the most elegant solution. On the other hand the feature is probably our most requested one, so we do need to address those requests somehow.

@vlazar
Copy link
Collaborator

vlazar commented Jan 13, 2016

Yes, I've noticed this is a frequently requested issue.

In fact, I've already added all key/value issues/PRs to my TODO list. Maybe I'm trying to be a perfectionist on this, but I think any API changes should be introduced very carefully.

To do refactoring and adding new features with more confidence I wanted to improve our test suite first. We have a lot of tests, but not enough yet as for me.

I've also started to work with small issues to be able to see the whole picture before I act on bigger issues as a collaborator.

Might try to address this feature first anyway, maybe it's not that big as it feels. Maybe an API change can be minimal.

@LeaVerou
Copy link
Owner

Absolutely agree that API changes need to be introduced very carefully. This is why I've been reluctant to merge this without very careful review.

@herregroen
Copy link
Author

Small new commit which partially reverses the only API change made.

Now the replace function is passed a second argument instead of a different one.

@ghost
Copy link

ghost commented Jan 13, 2016

Any idea how to remove oneself from this string? I believe this is email 375 from your group. Thanks

Sent from my iPhone; I apologize for any typographical or dictation errors.

On Jan 13, 2016, at 6:14 AM, Lea Verou notifications@github.com wrote:

@vlazar Any chance you could take a look at this? Don't merge, but I do need help reviewing as I can't find enough time to do it properly on my own. It seems to add a lot of code and I'm not sure it's the most elegant solution. On the other hand the feature is probably our most requested one, so we do need to address those requests somehow.


Reply to this email directly or view it on GitHub.

@TxHawks
Copy link
Contributor

TxHawks commented Jan 14, 2016

Just my two cents, I think this is a lot of code that is overly complicated and too opinionated for something that is already baked into awesomplete today through its amazing extensibility, pretty much out of the box.

The code is very elegant right now, and I think it would be a shame to add bloat to it unnecessarily.

I don't think that handling the hidden input is, or should be, Awesomplete's responsibility, as it only caters to one specific use-case of key:value pairs. Awesomplete's responsibility should be limitted to making the data available in the event object that's passed when awesomplete-select and awesomplete-selectcomplete are fired.

As I said, since f2020fa, Awesomplete already does this (in a bit of a buggy way, but that can easily be fixed, see my suggestion in #16795).

basically, to achieve the same result with the current codebase you'd need to have a hidden element in your form and do the following:

// Awesomplete init:
// (Assuming your list is an array of objects with 'name' and 'id' properties.)
new Awesomplete(awesomepleteInput, {
  filter: function(listItem, input) {
    return Awesomplete.FILTER_CONTAINS(listItem.name, input)
  },
  item: function(item, input) {
    var html = item.name.replace(RegExp(Awesomplete.$.regExpEscape(input.trim()), "gi"), 
                     "<mark>$&</mark>");
    return Awesomplete.$.create("li", {
      innerHTML: html,
      "data-id": item.id,
      "aria-selected": "false"
    });
  }
});

// The hidden input:
var hiddenInput = document.getElementById('hidden-input')

// Push the id to the hidden input:
awesomepleteInput.addEventListener('awesomplete-selectcomplete', function(e){
  hiddenInput.value = e.originalEvent.getAttribute('data-id');
}

And voila, the hidden input has the value passed in the id key, no extra code added to Awesomplete's core.

If you use this, just be aware that as of now, this only works when an item is selected with the mouse, not with the keyboard. But as I said before, this can easily be fixed.

@herregroen
Copy link
Author

I suppose it really depends on how you see Awesomeplete in greater context.

If it's just an awesome text-input with added functionality then it shouldn't manage a hidden input as that's clearly not part of the functionality.

If it's an awesome select input though then I think it should. Because that's what the select element also does in a way. After all, the value attribute on option elements allow you to save one value whilst displaying another. It doesn't require any additional setup to do this and as such Awesomplete should't either.

This pull request was written based on seeing it as an awesome select input. If instead I should see it as an awesome text-input then I'd agree that this PR goes beyond what's needed.

@TxHawks
Copy link
Contributor

TxHawks commented Jan 14, 2016

I completely agree with you that the hidden input is a valid use case and that it should be possible with Awesomplete.

However, this is only one of many use cases for key: value pairs, and I feel that, on one hand, setting up Awesomplete to work well with a hidden input is pretty trivial as it is, and that, on the other hand, the suggested solution binds Awesomplete to a single use case to strongly.

Just my two cents though.

@LeaVerou
Copy link
Owner

So far, I'm leaning towards @TxHawks' opinion.
I wonder if there's any smaller changes we could make to make this use case easier though.

@LeaVerou
Copy link
Owner

@Sundevil96 Not sure what string you're talking about, but sounds like you've accidentally somehow subscribed to the repo. Click Unwatch on the top right.

@vlazar
Copy link
Collaborator

vlazar commented Jan 15, 2016

The good thing is @herregroen's and @TxHawks's solutions are not mutually exclusive. We can have them both. The @herregroen's addresses an easy default setup for frequently requested key/value feature, while @TxHawks's code in recent state allows to manually add key/value feature, but it's only one of use cases for it.

After small cleanup I'm going to work with @TxHawks's PR. Then with @herregroen's, since it's much harder to come up with reasonable defaults for everyone.

@vlazar vlazar added this to the v1.1 milestone Jan 31, 2016
@vlazar
Copy link
Collaborator

vlazar commented Feb 15, 2016

To everyone interested in differentiating value from displayed text, aka key/value feature, aka array of objects feature.

The code is ready and it's almost 100% backward compatible. To ensure everything working fine for you we need your help with testing it in the wild before it's released. See this PR: Separate label/value for each suggestion in list

@vlazar
Copy link
Collaborator

vlazar commented Feb 15, 2016

@herregroen For example, now your request of having a select-like functionality can be implemented as simple as:

<input id="myinput-hidden" name="lang" type="hidden" />
<input id="myinput" />
var hidden = $("#myinput-hidden");

new Awesomplete("#myinput", {
    list: [["Ada", 1], ["Java", 2], ["JavaScript", 3], ["Brainfuck", 4], ["LOLCODE", 5], ["Node.js", 6], ["Ruby on Rails", 7]],

    // the only function you need to override:
    replace: function(suggestion) {
        this.input.value = suggestion.label; // default replace() inserts suggestion.value to input
        hidden.value = suggestion.value;
    }
});

@vlazar
Copy link
Collaborator

vlazar commented Mar 12, 2016

Separate label/value is landed #16866
It is also possible to initialize list with separate label/value via <datalist> or <ul>.

Read about different label and value at the end of Basic usage section and about new data method at the end of Extend section.

@vlazar
Copy link
Collaborator

vlazar commented Mar 13, 2016

I suppose it really depends on how you see Awesomeplete in greater context.

If it's just an awesome text-input with added functionality then it shouldn't manage a hidden input as that's clearly not part of the functionality.

If it's an awesome select input though then I think it should. Because that's what the select element also does in a way. After all, the value attribute on option elements allow you to save one value whilst displaying another. It doesn't require any additional setup to do this and as such Awesomplete should't either.

There is now a separate ticket for out of the box <select> support #16789. We haven't discussed it yet, but maybe another advanced example instead of out of the box support is what it will end with.

@vlazar vlazar closed this Mar 13, 2016
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.

7 participants