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/pass target to awesomplete select #16819

Conversation

vlazar
Copy link
Collaborator

@vlazar vlazar commented Jan 16, 2016

Addresses #16818

I'm not happy with option named target though.

Since an awesomplete-select is fired on INPUT, inside it this, event.target and event.currentTarget are references to INPUT originally. In click target is a reference to clicked element, but in awesomplete-select it may be misleading.

Eventually we need to pass 2 elements to awesomplete-select event for new features:

  • first one is in this request - exact clicked item (child of LI or LI itself)
  • second is always LI (selected element)

I can't come up with good aligned names for those 2 new fields in awesomplete-select event.

Any ideas?

@TxHawks
Copy link
Contributor

TxHawks commented Jan 16, 2016

I think that

  1. both target and selected should be passed to awesomplete-select, though I guess it's not a must, since you could always get to it using target.parentNode() or target.closest().
  2. awesomplete-selectcomplete should also be privy to target (and possibly selected, if it's added so awesomplete-select

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 17, 2016

@TxHawks

  1. I've been thinking about using target and selected names, except that target is INPUT originally. Personally I'm leaning towards passing both, since selected is essential and target is nice to have for advanced use. Also IE/Edge currently is lacking target.closest() and until awesomplete goes with polyfills it may be not an option to keep this for users to deal with.
  2. Seems your text is cut. I agree that other event's options should be reviewed, and we need to pass new options where it makes sense).

With this PR and your #16795 landing it will be easier to do a frequently requested key/value (aka arrays of objects) feature among other things.

@TxHawks
Copy link
Contributor

TxHawks commented Jan 17, 2016

@vlazar

It wasn't cut, just had a typo. The last so should have been to.

Tjanky you for doing all this work, by the way

@LeaVerou
Copy link
Owner

Maybe an item property for the LI? Not sure, need to think about this more.

Btw, minor code nitpick: Please don't use variables when they are only used once. It's an indirection for humans reading the code (as they need to pointlessly look up the variable definition) and bloats the code size too. In this case, using evt.target in both lines would have been fine, no need for a target variable just to save one property reference. I know it's supposed to be marginally faster, but it's premature optimization and readability matters more.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 19, 2016

Maybe an item property for the LI? Not sure, need to think about this more.

I've been thinking about using item later as a good property name for item data itself, when items are objects instead of strings. We have text property for item data now, but this name only makes sense when each item is a string. Would be nice change or addition in my opinion.

In this case, using evt.target in both lines would have been fine, no need for a target variable just to save one property reference.

I'll change this. Wasn't intended as a way to save a property reference though. The target and li are conceptually different. The li just starts from target originally and became guaranteed li in a while loop later going up the DOM tree. We might want to replace this with element.closest() polypill later if polyfills are an option as in Bliss.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 20, 2016

How about an origin property instead of target for exact clicked element (child of LI or LI itself)? It's like target element in mousedown event becomes origin element of awesomplete-select event.
cc @TxHawks

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 20, 2016

Maybe origin for exact clicked element and element for selected LI element?

The more I think about it, item property with item data seems like the most useful property to add. I guess my current vision of dream API:

// existing option; I prefer to replace/deprecate it in 2.0 with next option
text: itemText,
// most useful property (text when items are strings, object if items are array of objects)
item: itemTextOrItemObject,
// allows to do something with selected element
// less useful, especially if item property above added
element: selectedItemElement,
// very specific use case
// the least useful, if at all
// maybe there is a better way for original use case for added `originalEvent` option
origin: clickedChildOfItemElement

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 21, 2016

@LeaVerou Can you please look at PR in the current state, and if you can't think of better name I'll clean this up and merge?

And my comment above #16819 (comment) with possible properties for next PRs.

I think item would be great addition. For 2.0 it can be even a replacement for text. We can pass it everywhere we use text property or argument. It aligns well with having a list items in different formats (e.g. objects in addition to strings we have now).

@LeaVerou
Copy link
Owner

I'm a bit confused with all the PRs, but I just left a comment in the other one.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 22, 2016

Sorry for confusion. Just let me know if this PR should be merged. This PR is only about 2 & 3 as you already agreed in #16818 (comment)

@LeaVerou
Copy link
Owner

Sure, this is fine.

@vlazar
Copy link
Collaborator Author

vlazar commented Jan 23, 2016

Closing in favor of #16823

@vlazar vlazar closed this Jan 23, 2016
@vlazar vlazar deleted the feature/pass-target-to-awesomplete-select branch January 29, 2016 11:57
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

3 participants