Skip to content

add key value support - #16774

Closed
herregroen wants to merge 8 commits into
nudeui:gh-pagesfrom
herregroen:gh-pages
Closed

add key value support#16774
herregroen wants to merge 8 commits into
nudeui:gh-pagesfrom
herregroen:gh-pages

Conversation

@herregroen

Copy link
Copy Markdown

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

SkeLLLa commented Oct 26, 2015

Copy link
Copy Markdown

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

@herregroen

Copy link
Copy Markdown
Author

Added a test for the new functionality.

If desired I can create similar tests for existing functionality.

@davycheung

Copy link
Copy Markdown

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

@LeaVerou

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

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

@ricardobanegas

Copy link
Copy Markdown

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

@herregroen

Copy link
Copy Markdown
Author

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

@LeaVerou

Copy link
Copy Markdown
Collaborator

@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

vlazar commented Jan 13, 2016

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

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
Copy Markdown
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

ghost commented Jan 13, 2016

Copy link
Copy Markdown

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

TxHawks commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

TxHawks commented Jan 14, 2016

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Collaborator

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
Copy Markdown
Collaborator

@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

vlazar commented Jan 15, 2016

Copy link
Copy Markdown
Collaborator

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

vlazar commented Feb 15, 2016

Copy link
Copy Markdown
Collaborator

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

vlazar commented Feb 15, 2016

Copy link
Copy Markdown
Collaborator

@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

vlazar commented Mar 12, 2016

Copy link
Copy Markdown
Collaborator

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

vlazar commented Mar 13, 2016

Copy link
Copy Markdown
Collaborator

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