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

feat(typeahead): support for grouped results (#1274) #3711

Open
wants to merge 1 commit into
base: master
from

Conversation

@gpolychronis
Copy link
Contributor

@gpolychronis gpolychronis commented Apr 30, 2020

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md and DEVELOPER.md guide.
  • built and tested the changes locally.
  • added/updated any applicable tests.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.
@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Apr 30, 2020

@maxokorokov Please review

Copy link
Member

@benouat benouat left a comment

I left a few comments here.

Overall, I need to think more about this feature. We've done to many times wrong API design choices on the typeahead by the past that we need to put extra effort into that one.

@@ -116,6 +184,8 @@ export class NgbTypeaheadWindow implements OnInit {

select(item) { this.selectEvent.emit(item); }

getContext() { return this; }

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

I am not keen of opening this kind of hole. Not for public API.

This comment has been minimized.

@maxokorokov

maxokorokov May 18, 2020
Member

It is not the "public" API, but it is required to pass the 'window' to the 'window template' context and then to the 'window item' as an input.
It is a public method, but not a public API (same as .select() method).

https://github.com/ng-bootstrap/ng-bootstrap/pull/3711/files#diff-2b05df9eab924379f039993fae7b433eR101

That is the biggest bottleneck in the PR, but I've no idea how to solve it...

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

My point here is to not return this !! We return an object containing only what we need to access.

getContext() {
  return {
    markActive: ...
    select: ... 
    id: ...
    activeIdx: ...
    resultTemplate: ...
  }
}

If we need them live we could keep getters so they are executed when accessed, but we don't return this. To me, this is not safe/maintainable.

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

Other solution, we could also pass every single needed property as one on the templateContext itself, instead of forwarding this

Then people would write

<.  ..... let-markActive="markActive" let-select="select"  .... >

This comment has been minimized.

@maxokorokov

maxokorokov May 18, 2020
Member

I don't think there was any intention to give this kind of API for people. What's the point, if we handle all this internally?

We just need a reference to the window (that's how it works today at least) from an item.

This comment has been minimized.

@maxokorokov

maxokorokov May 18, 2020
Member

On the other hand I see what you mean. If we have this API, we could use it and then people could use it. And it might be cleaner overall.

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

Let me rephrase my concern here:

👍 I am not against the surface API provided by an input @Input() context on the component.

👎 I am against providing this as a value to it. this will move over time, it's opaque, you don't know what it provides. Forcing us to declare what we export as context makes us design better API.

And finally, for pure documentation, we should use an interface to declare this context, to avoid such any effect as shown below

image

This comment has been minimized.

@gpolychronis

gpolychronis May 19, 2020
Author Contributor

Let me rephrase my concern here:

👍 I am not against the surface API provided by an input @Input() context on the component.

👎 I am against providing this as a value to it. this will move over time, it's opaque, you don't know what it provides. Forcing us to declare what we export as context makes us design better API.

And finally, for pure documentation, we should use an interface to declare this context, to avoid such any effect as shown below

image

I would like to make context not visible by users at all. We would like to change what fields belong to context instead of exposing them to users. The only problem is how to inject context in items without users noticing. It isn't meant to be used by users, that's why it has the generic "any". It should not be used by users at all.

@Component({
selector: 'ngbd-typeahead-grouping-internal',
template: `
<div *ngFor="let category of categories">
<h3>{{category.label}}</h3>
<div *ngFor="let item of category">
<ngb-typeahead-window-item
[result]="item.label"
[index]="item.idx"
[context]="context">
</ngb-typeahead-window-item>
</div>
</div>
`
})
export class NgbdTypeaheadGroupingInternal {
categories: any[];

@Input()
context: any;

@Input()
set results(results: any[]) {
let references = {};
let categories: any[] = [];
let idx = 0;
for (const result of results) {
const category: string = result.toUpperCase().charAt(0);
if (!references[category]) {
references[category] = categories.length;
const c = [];
(c as any).label = category;
categories.push(c);
}
categories[references[category]].push({
label: result, idx: idx++
});
}
this.categories = categories;
}
}
Comment on lines 36 to 66

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

I think this should be extracted as a standalone file, referenced in the demo. It's not clear for user that the demo contains this component definition. It would appear in the Code tab list

This comment has been minimized.

@maxokorokov

maxokorokov May 18, 2020
Member

I would add that results should not be grouped in the component, but in the search service directly. Component NgbdTypeaheadGroupingInternal should just do the display. Also the component should be renamed...

This comment has been minimized.

@gpolychronis

gpolychronis May 19, 2020
Author Contributor

There is the issue with change detection. So a new component cannot be avoided. I will try to put in a separate file though.

templateUrl: './typeahead-grouping.html',
styles: [`.form-control { width: 300px; }`]
})
export class NgbdTypeaheadGrouping {

This comment has been minimized.

@benouat

benouat May 18, 2020
Member

I think our US list of states is not relevant for that kind of demo.
We could inspire ourselves from official Twitter Typeahead.js multi-datasets example.

This comment has been minimized.

@maxokorokov

maxokorokov May 18, 2020
Member

The demo is also broken if you type 2 letters, like "Co" (Colorado, Connecticut). They will be split in 2 groups

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

My previous comment here is still valid to me, we should think about another problem domain for this demo. List of states is not good...

@maxokorokov
Copy link
Member

@maxokorokov maxokorokov commented May 18, 2020

Also, @gpolychronis, please explain things in future when opening PRs in the description. It helps to understand what is done / not done, what was your thinking, etc. It should be clear what you're trying to do without looking into the code.

Ex. what is that you're introducing, what is the public API, etc.

cc @benouat

@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented May 18, 2020

Also, @gpolychronis, please explain things in future when opening PRs in the description. It helps to understand what is done / not done, what was your thinking, etc. It should be clear what you're trying to do without looking into the code.

Ex. what is that you're introducing, what is the public API, etc.

cc @benouat

I would like to enable user to write something like that:

<div *ngFor="let category of categories">
        <h3>{{category.label}}</h3>
        <div *ngFor="let item of category">
          <ngb-typeahead-window-item
            [result]="item.label"
            [index]="item.idx"
            [context]="context">
          </ngb-typeahead-window-item>
        </div>
      </div>


I don't like context either. I would like it not be visible to user at all. However as metnioned, the problem remains. How will typeahead instance be injected in items? Any ideas?

@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented May 20, 2020

This is the blocking issue: angular/angular#14935

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from 6b7c57f to 776271e May 25, 2020
Copy link
Member

@benouat benouat left a comment

@gpolychronis Have a look at TravisCI output, there is a circular dependency in your code. I don't know if the failing UT is due to that, most probably yes.
You'll have to use forwardRef I think to overcome this. Search for it on the project to find use case usage of it.

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from 776271e to e6db91b May 26, 2020
@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented May 26, 2020

@benouat It seems there is an error on Travis side. Am I right, or there is an error on my side? Please chec.

@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented May 27, 2020

UPDATED DESCRIPTION:

In order to enable result grouping, a new template has been added. The new template allows user to customize the layout of the typeahead results.

Technically, there is a problem. The NgbTypeaheadWindow instance must be injected into the items. Right now ngbTypeahead is bound to input element. Input element has no closing tag, so injection via html is not possible.

The solution provided is this: untie ngbTypeahead from input. This way ngbTypeahead can be used with a div element. The layout template can be included inside the this div as long as the input itself.

At the same time the old way shall be supported too. So ngbTypeahead checks if there is any input child, else it assumes bound element is the input one. Also, if ngbTypeahed is bound to a div, then this div is supposed to include only the input, its label and the layout template.

@maxokorokov @benouat Please review

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from e6db91b to 4f0a9a7 Jun 11, 2020
@codecov-commenter
Copy link

@codecov-commenter codecov-commenter commented Jun 11, 2020

Codecov Report

Merging #3711 into master will decrease coverage by 0.12%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3711      +/-   ##
==========================================
- Coverage   91.23%   91.10%   -0.13%     
==========================================
  Files         100      100              
  Lines        2988     3013      +25     
  Branches      555      558       +3     
==========================================
+ Hits         2726     2745      +19     
- Misses        192      194       +2     
- Partials       70       74       +4     
Flag Coverage Δ
#e2e 56.22% <63.63%> (-0.01%) ⬇️
#unit 87.65% <81.81%> (-0.10%) ⬇️
Impacted Files Coverage Δ
src/index.ts 100.00% <ø> (ø)
src/typeahead/typeahead.module.ts 100.00% <ø> (ø)
src/typeahead/typeahead.ts 92.99% <81.25%> (-3.25%) ⬇️
src/typeahead/typeahead-window.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dc7990f...4f0a9a7. Read the comment docs.

@benouat
Copy link
Member

@benouat benouat commented Jun 11, 2020

@gpolychronis what is this last force push about ?

@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Jun 11, 2020

@gpolychronis what is this last force push about ?

An attempt to rebuild and check again the tests. It worked.

@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Jul 8, 2020

  1. Test if it fails gracefully if misused.
  2. New PR: update all demo examples.
  3. Replace @ViewChild('input', {static: true}) childInput: ElementRef; with query
    4a. "ngb-typeahead-window-item" -> "ngb-typeahead-item"
    4b. "ngb-typeahead-window-item".result -> "ngb-typeahead-item".item
@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Aug 7, 2020

1. Test if it fails gracefully if misused.

2. New PR: update all demo examples.

3. Replace  @ViewChild('input', {static: true}) childInput: ElementRef; with query
   4a. "ngb-typeahead-window-item" -> "ngb-typeahead-item"
   4b. "ngb-typeahead-window-item".result -> "ngb-typeahead-item".item

Done. @maxokorokov , @benouat please review.

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from 4f0a9a7 to bb99110 Aug 7, 2020
Copy link
Member

@benouat benouat left a comment

I have seen a lot of progress in the PR 👍
I have reviewed things again and do have some remarks and comments I left here and there.
Please @gpolychronis have a look a them

<div *ngFor="let slide of slides; index as i; count as c" class="carousel-item"
[class.active]="slide.id === activeId" [id]="slide.id + '-slide'" role="tabpanel">
Comment on lines +74 to +75

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

This is not related to this PR, probably just a formatting issue, changes should be discarded.

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

CI cannot work otherwise

@@ -45,7 +48,7 @@ const NGB_TYPEAHEAD_VALUE_ACCESSOR = {
*/
export interface NgbTypeaheadSelectItemEvent<T = any> {
/**
* The item from the result list about to be selected.
* The item from the item list about to be selected.

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

Maybe simplify it * The item from the list about to be selected.

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

set autocomplete(_autocomplete: string) {
this._autocomplete = _autocomplete;
if (this._inputRef) {
this._inputRef.nativeElement.setAttribute('autocomplete', this._autocomplete);

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

You should use the Renderer here. Without it, code will crash in environment which are not browsers (i.e. SSR for example). We already have it from the constructor as this._renderer

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

* If `true`, the first item in the item list will always stay focused while typing.
*/
@Input() focusFirst: boolean;

/**
* The function that converts an item from the result list to a `string` to display in the `<input>` field.
* The function that converts an item from the item list to a `string` to display in the `<input>` field.
Comment on lines 123 to 128

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

See my previous comment on simplifying the wording a bit here
* If true, the first item in the list will always ...
* The function that converts an item from the list to a ...

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

@@ -138,7 +146,7 @@ export class NgbTypeahead implements ControlValueAccessor,
@Input() ngbTypeahead: (text: Observable<string>) => Observable<readonly any[]>;

/**
* The function that converts an item from the result list to a `string` to display in the popup.
* The function that converts an item from the item list to a `string` to display in the popup.

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

Same as previous

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

</button>
`
})
export class NgbTypeaheadItem {

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

It feels that we are missing a Generic here for the class. Either <T> or something from the ContentTemplateContext class

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

/**
* The context for the typeahead content template in case you want to override the default one.
*/
export interface ContentTemplateContext {

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

This context class should be somehow using Generics like ContentTemplateContext<T> where results: T[]

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

[ngTemplateOutletContext]="{result: result, term: term, formatter: formatter}"></ng-template>
</button>
<ng-template #defaultContentTemplate let-r="results">
<ng-template ngFor [ngForOf]="r" let-item let-idx="index">

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

Please use let-results="results".
Same for let-index="index". We don't use that kind of shortcut names such as idx in the project

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

</ng-template>
<ng-template [ngTemplateOutlet]="contentTemplate || defaultContentTemplate"
[ngTemplateOutletContext]="{results: results}"></ng-template>

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

You could use here in addition to results the $implicit key. Simply duplicate thing.

{
    $implicit: results,
    results: results
}

This comment has been minimized.

@gpolychronis

gpolychronis Oct 5, 2020
Author Contributor

done

templateUrl: './typeahead-grouping.html',
styles: [`.form-control { width: 300px; }`]
})
export class NgbdTypeaheadGrouping {

This comment has been minimized.

@benouat

benouat Aug 25, 2020
Member

My previous comment here is still valid to me, we should think about another problem domain for this demo. List of states is not good...

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch 2 times, most recently from 0f5f98f to 413c091 Oct 5, 2020
@gpolychronis
Copy link
Contributor Author

@gpolychronis gpolychronis commented Oct 5, 2020

Just a note: One typeahead template variable "result" is renamed to "item". This will probably break backwards compatibility.

@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from 413c091 to 9957af0 Oct 5, 2020
@gpolychronis gpolychronis force-pushed the gpolychronis:feature/1274 branch from 9957af0 to acadcfd Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.