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

Feature/handle array of objects #16795

Closed

Conversation

TxHawks
Copy link
Contributor

@TxHawks TxHawks commented Dec 9, 2015

Addresses #16788 by attaching the selected item's dataset to the event object that is passed when the awesomplete-select and awesomplete-selectcomplete events are fired.

In cases where out list is an array of objects, this allows adding data-* attributes to the lis when building the items based on keys in the object, and then accessing them when the awesomplete-select and awesomplete-selectcomplete events are fired.

Pass the selected item's dataset to the `awesomplete-select`
and `awesomplete-selectcomplete` events to allow firing actions
based on secondary keys in the object
Merge branch 'gh-pages' into feature/handle-array-of-objects
- Fix bug created in LeaVerou#16794, which caused different behaviours
when selecting an item with the keyboard and with a mouse.
@TxHawks
Copy link
Contributor Author

TxHawks commented Dec 28, 2015

Not sure why the tests are failing - when I test in the browser locally, it all seem to work just fine...

@TxHawks
Copy link
Contributor Author

TxHawks commented Jan 2, 2016

@vlazar -

Sorry to bug you, but could you maybe take a look and see why the tests are failing? I tried, but didn't really manage to figure it out.

Thanks!

@vlazar
Copy link
Collaborator

vlazar commented Jan 2, 2016

@TxHawks I saw the call signature was changed from me.select(); to me.select(li, evt); and it looks like failing tests are calling this method with 1 argument or no arguments at all. So originalTarget would be accessed on undefined.

With changes you've made... Does the select() API method works as expected (as described in docs) with 1 argument or without any arguments for you?

@vlazar
Copy link
Collaborator

vlazar commented Jan 2, 2016

I probably was not clear enough. What I was referring to is this line https://github.com/TxHawks/awesomplete/blob/feature/handle-array-of-objects/awesomplete.js#L205

The originalEvent is expected to be passed as a second argument to the select() public API method. That's not always the case.

However I think this second argument is already quite a controversial feature to have. Why someone might need to pass originalEvent to the public API method in the first place?

Maybe #16794 was a nice addition, but should it really change the public API? We might want to hear @LeaVerou opinion on this matter.

@TxHawks
Copy link
Contributor Author

TxHawks commented Jan 2, 2016

@vlazar -

You are absolutely right. I changed the code to that the event is optional again.

@TxHawks
Copy link
Contributor Author

TxHawks commented Jan 9, 2016

@LeaVerou - Sorry to nag, but any chance you can look at this #16796 and #16805? I need them for a project and don't like diverging from the maintained upstream.

Thanks!

No need to modify the event object, the selected element just needs to be
passed to the fired events in `select()`.

arrOfObj.addEventListener('awesomplete-selectcomplete', function(e){
// Do something with e.originalTarget.getAttribute('data-*')
console.log(e.originalTarget.getAttribute('data-url'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be a good idea to do something visual here, instead of doing console.log. One obvious example would be to use only URL property of item as input value when selected.

Unfortunately, having selected element passed into awesomplete-selectcomplete here in e.originalTarget allows only a hacky way to use different value for input when selected.

We can change input value:

e.target.value = e.originalTarget.getAttribute('data-url');

But this will happen after an input value was already changed to selected.textContent by awesomplete itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think that changing the input value in the event isn't good practice, as it can be easily done in the item function to begin with.

I do agree that console.log() doesn't exactly showcase this ability very well though. I'll try and think of something better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strike what I said in the previous comment, it's absolutly rubbish.

Note to self: do not write when sleep deprived.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understood you actually meant replace function, where the input value is assigned from selected text now. Having item instead of text in replace would make it possible to assign either item's key or value or some combination of them to input.value.

@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

@TxHawks
Copy link
Contributor Author

TxHawks commented Feb 15, 2016

Will do.

Was following the thread(s) on #16852 and it looks pretty solid

@TxHawks
Copy link
Contributor Author

TxHawks commented Feb 15, 2016

@vlazar - could you post a comment summarize everything that changed in #16852? There were a few back and forths there on a few issues, and it could make everything less confusing.

Thank you again for all the work you are putting into this.

@vlazar
Copy link
Collaborator

vlazar commented Feb 15, 2016

@TxHawks Just let me know which part of #16852 description sounds confusing. I'll try to explain better.

@TxHawks
Copy link
Contributor Author

TxHawks commented Feb 15, 2016

You wen back and forth on __proto__ and how Suggestion will work, so you really need to read through everything to understad what you ended up going with. I did read it all, so I'm clear on that, but if someone wasn't following too closely, it may be harder to grasp. So maybe a summarizing comment on these two things could be good.

@vlazar
Copy link
Collaborator

vlazar commented Feb 15, 2016

Gotcha! We only care about external API, not internal implementation. But it's a good idea, I'll add a known issue with having Suggestion instead of just text as before.

@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 16, 2016

Just a thought: I think we still need an advanced example based on yours. Like a small bookmark manager maybe?

@vlazar vlazar modified the milestones: V1.2, v1.1 Mar 16, 2016
@vlazar
Copy link
Collaborator

vlazar commented Sep 26, 2016

Closing this one since array of objects case is already supported.

@vlazar vlazar closed this Sep 26, 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.

2 participants