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

NgbTypeahead: Open Function #2318

Open
f-aubert opened this issue Apr 16, 2018 · 9 comments
Open

NgbTypeahead: Open Function #2318

f-aubert opened this issue Apr 16, 2018 · 9 comments

Comments

@f-aubert
Copy link

As many others here, I have the "List all options on focus" Use Case. I just attempted to use the focus trick with a new rxjs Subject, as explained in the documentation, and I don't think it's convenient at all. One should not have to use extra rxjs Subjects to handle this Use Case, and it makes it particularly annoying when we generate many such Typeahead Fields in a ngFor loop, for example a list of states, as for now we would need to generate one such rxjs Subject for each loop iteration.

Would it be possible to implement open (close, toggle) methods in the same way as what was already done for the NgbDatePicker? So we could simply decide when to call them (for example referring the element, and calling open in the focus event handler).

Thanks a lot for considering this.

Version of Angular, ng-bootstrap, and Bootstrap:

Angular: 5.2.x
ng-bootstrap: 1.0.x
Bootstrap: 4.0.x

@pkozlowski-opensource
Copy link
Member

Yep, the initial impl was done so people are not blocked and can handle the "open on focus" use-case. But I agree that we could make it more convenient for users. Marking as a feature request. This will need design / API idea(s), though.

@pkozlowski-opensource
Copy link
Member

@f-aubert I was thinking a bit about design for the open(...) method on a typeahead. What I'm pondering right now are arguments that should be passed to this new method. I can think of 3 options:

  1. open(resultsToDisplay)
  2. open(searchTerm)
  3. have 2 methods (startSearch(searchTerm) and openWithResults(resultsToDisplay).

To better illustrate those 2 approaches with our simple state-search example, the difference between (1) and (2) would be:

Option (1) - open with results

const states = ['Alabama', 'California', ...];

typeahead.open(states);

This would immediately open a typeahead window and display passed-in results (and replace displayed results if a window is already opened).

Option (2) - open with a search term

typeahead.open('');

This would trigger the existing search machinery and eventually open a window with results matching the term.

I still need to go over all the pros, cons and consequences of each approach but I wanted to reach out and check what would make most sense in your usage scenario(s).

@f-aubert
Copy link
Author

For me, as I'm mainly concerned with showing all possible choices on focus, option 2 is what I need! Option 1 could be nice to have but could be simulated more easily. Usually you want to show everything when no text is in the input.

@f-aubert
Copy link
Author

Hi, do you see a conceptual or technical problem with option 2? Is there anything I can do to help brainstorming, or implementing? All my best.

@pkozlowski-opensource
Copy link
Member

@f-aubert yeh, actually I've got a conceptual problem with the option (2). Let me try to explain it.

If we assume that option (2) should behave like if a user typed something into an input field that the argument supplied to the open('search term') should go through the same observable pipeline as any other user input. But it poses 2 problems with the standard observables pipeline we are recommending users to use:

  • debounceTime - having this in a pipeline would mean that the open('search term') doesn't take effect immediately but only after some delay;
  • distinctUntilChanged - this means that opening typeahead with the exact same search term would work only once and not the second time.

The only idea I've got right now is to change typeahead implementation and move both debounceTime and distinctUntilChanged into inside of the typeahead (so users wouldn't have to specify it). This would be a breaking change, unfortunately...

If you've got different ideas / suggestions / impl proposals I would love to hear from you! We want to make it "right" for our users but it is not obvious for me what this "right" should be...

@f-aubert
Copy link
Author

f-aubert commented May 25, 2018

You are absolutely right, I now don't see how option 2 could be implemented without the suggested breaking change.

So maybe until you allow a breaking-change to take place option 1 would help as well. On the side note, I think pretty much everyone should be using debounce and distinctUntilChanged, which make the most sense. Angular own material-ui probably does something similar. So I guess such change wouldn't be counter-intuitive.

As reference, here is my typical usage at the beginning of the typeahead Function, which filter the second Array Observable with the value of the first
return combineLatest(text$.pipe(debounceTime(200), distinctUntilChanged()), this.locations$)

which might translate to something like this (out of memory as I don't have my dev env with me)
<input id="location" name="location" formControlName="location" class="form-control" type="text" (focus)="instance.show(locations$ | async)" [ngbTypeahead]="locationsTypeahead" #instance="ngbTypeahead">

In this case option 1 (named 'show') could be more easily implemented now, leaving option 2 (named 'trigger') for a time where you would allow yourself a breaking change (in version 3.0?). What are your thoughts?

@Rhobal
Copy link

Rhobal commented Aug 4, 2018

I have solved the issue for myself by creating an additional directive that triggers input events on click or focus, as input events trigger the opening of the typeahead selection popup.

import { Directive, HostListener } from '@angular/core'
import { NgbTypeahead } from '@ng-bootstrap/ng-bootstrap';

@Directive({
  selector: 'input[ngbtypeaheadpopup]'
})
export class TypeaheadPopupDirective {
  @HostListener('focus', ['$event.target'])
  @HostListener('click', ['$event.target'])
  onClick(t) { if (!this.typeahead.isPopupOpen()) t.dispatchEvent(new Event('input')) }

  constructor(private typeahead: NgbTypeahead) {
  }

}

The drawback of this method is that the popup waits for the debounce time, while the example in the documentation skips debounce time for clicks and focus. Thus, I only use debounce time if the term length is >0 in my search by using a conditional debounce() instead of debounceTime():

search = (text$: Observable<string>):Observable<string[]> => {
   return text$.pipe(
     debounce(t => timer(t.length > 0 ? 200:0)),
     distinctUntilChanged(),
     map(term => term === '' ? states
       : states.filter(v => v.toLowerCase().indexOf(term.toLowerCase()) > -1).slice(0,20))
   )};

When the field is not empty on focus or click, the 200ms delay remains. To me that is a trivial drawback I'll live with.
Stackblitz

@dexluce
Copy link

dexluce commented Nov 19, 2020

Hello @Rhobal,

Have you tried your directive with a reactive form, the parameter "Editable" set to false, and a object as a value?

        <input
          appNgbTypeaheadOpenOnFocus
          class="form-control"
          type="text"
          formControlName="MyControl"
          [ngbTypeahead]="$search"
          [inputFormatter]="$inputFormatter"
          [resultFormatter]="$resultFormatter"
          [editable]="false"
        />

In that configuration, if I populate my control with a value, the value appear correctly, but the focus event from the directive clear the model.
So on focus, the input show the correct value, but the control has an undefined.
With a validator, the input goes to "invalid" on focus even if the initial value is correct.

I'll add a working example if needed but you may have an idea to fix that?

@Rhobal
Copy link

Rhobal commented Dec 2, 2020

No, my configuration works well enough that I don't worry about it anymore. Judging by the thumbs, it also works from ome others...

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