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 (Typeahead): Keydown.Enter without focused item to trigger selectItem event #877

Closed
kjartanvalur opened this issue Oct 12, 2016 · 13 comments

Comments

@kjartanvalur
Copy link

Use case:
Specially when the new config focusFirst is set to false then users will sometimes just want to press enter although no item is selected/focused. Then most of the time the behaviour is to redirect to search result page.

Solution:
SelectItem event can be triggered with value.item = null when enter key is pressed and no item is selected/focused.

@pkozlowski-opensource
Copy link
Member

@kjartanvalur I'm sorry but I don't think I understand the use-case. Could you elaborate? More specifically this part is not clear at all:

Then most of the time the behaviour is to redirect to search result page.

Sorry for being pain in a neck but I want to make sure that I understand the use-case before we change the API.

@kjartanvalur
Copy link
Author

@pkozlowski-opensource, sure I understand.

What I´m trying to do is a autosuggest search input on remote datasource (>100k items) and I also configure selectFirst=false. So when a user types in a search query and does not select from the auto suggest results and hits the Enter key, then I redirect to a search page with that search text he has entered. This is somewhat a standard behaviour in all search inputs, like e.g. amazon, imdb etc.

The problem is that if i configure (keyup.enter)="navigateToSearch(searchText)" then it conflicts with the enter event in the typeahead control that triggers onSelectedItem.

@wesleycho
Copy link
Member

This sounds like a reasonable request - this would be an easy thing to create a PR, maybe with input autoselect. If false, then the enter or tab keydown should do nothing.

@kjartanvalur
Copy link
Author

@wesleycho
Don´t you think it´s better if the SelectItem event would just trigger but with parameter item = null, given nothing is selected. That way select with arrowkey and enter would stilll work normally.

@deeg
Copy link
Contributor

deeg commented Oct 18, 2016

I am running into the same use case, and can put out a PR to fix this.

I just want to make sure we are all on the same page with the resolution before doing so.

As far as I can tell there are a few solutions:

  • When pressing enter and nothing is selected from the dropdown (focusFirst=false), fire the selected event anyways, but make event.item = null;
  • When pressing enter and nothing is selected, do not run event.preventDefault so that the user can bind their own keydown event without ng-bootstrap's stopping it.
  • Create new input [autoselect], which if false tab/enter do nothing.

@wesleycho, the third option there is I believe what you were trying to say, but I'm not sure that would work as well. It would require the user to then handle the selection event themselves.

@deeg
Copy link
Contributor

deeg commented Oct 18, 2016

If it helps anyone out, as a temporary work around, you can bind to the keyup event of the input instead of keydown or keypress

@kjartanvalur
Copy link
Author

@deeg Hi. It would be awesome if you could put out a PR 👍
I think the first solution is best

@deeg
Copy link
Contributor

deeg commented Oct 20, 2016

I started going down the path of the first solution and realized there are some issues with it:

  • This line indicates that the keyboard select event won't happen until the typeahead window is up. This means if a user just wants to preform a search, and doesn't care about seeing results, they have to wait until the results come back from server to fire the select function. This also means if there are no results for the typeahead for that search, the select function will never fire
    • We could easily remove that check and run the select function anyways, but we definitely want that check there for the other actions.
  • No access to the target object. I like to call blur on the target for the input, so the user is no longer focused on the search input. It would be easy to pass along, but I'm not sure if this wasn't done intentionally, as they don't pass along the entire object. @pkozlowski-opensource, might know?

I'm going to wait what people think about the above, and think on a good solution and wrap up the PR. For now I'm finding using the keyup event works very well for me right now.

@pkozlowski-opensource
Copy link
Member

@deeg so I'm reviewing typeahead related issues and after re-reading #980 I'm not sure anymore if firing selectItem when nothing is selected is the best approach. Somehow it feels like we are blurring semantic of this event which should be really only called when a user is selecting an item from typeahead results.

How about we do the following:

  • selectItem should not be fired when nothing is selected
  • we do not do preventDefault() on keydown.enter when there is nothing selected
  • we do preventDefault() and stopPropagation() when there are things to select.

This way we could keep selectItem for real results selection and people could use keyup.enter for their custom handling when no result gets selected.

I'm still not sure if the above proposal covers all use-cases described here and in #980 so another pair of eyes would help clarify things.

cc @stevewarsa

@kjartanvalur
Copy link
Author

@pkozlowski-opensource
This may work. But what is the recommended way of deselecting an item. In my opinion it makes sense to call selectItem also when nothing is selected

@pkozlowski-opensource
Copy link
Member

But what is the recommended way of deselecting an item

Not sure what you mean by "deselecting an item"...

@deeg
Copy link
Contributor

deeg commented Nov 10, 2016

@pkozlowski-opensource, I'm not sure of the difference between this ticket and #980.

I feel like both solutions will work for both tickets, and I'm fine with going the route you just proposed above.

  • selectItem should not be fired when nothing is selected
  • we do not do preventDefault() on keydown.enter when there is nothing selected
  • we do preventDefault() and stopPropagation() when there are things to select.

I can change the PR I have open later tonight or tomorrow.

@kjartanvalur
Copy link
Author

Not sure what you mean by "deselecting an item"...

I mean when a user already selected an item and he wants to clear selected item (e.g. reset form). I was just wondering what would be the preferred way and maybe it would conflict with this resolution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants