-
Notifications
You must be signed in to change notification settings - Fork 610
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
add key value support #16774
Conversation
Done almost the same thing today. It would be nice to have such functionality. |
Added a test for the new functionality. If desired I can create similar tests for existing functionality. |
I see this hasn't been merged, any status on this one? Key value support would be splendid, thx. |
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! |
Agree! Would be very good if key-value support was added |
Any news on this PR? I've been using it for my project and works like a charm. |
Just updated my pull request to the latest version since it seems I'm not the only one using it. |
@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. |
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. |
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. |
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. |
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.
|
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 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. |
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. |
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. |
So far, I'm leaning towards @TxHawks' opinion. |
@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. |
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. |
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 |
@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;
}
}); |
Separate Read about different |
There is now a separate ticket for out of the box |
Adds support for key value pairs in the form of an array of pairs.
Key is added to the form through a hidden input.