Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upfeat(typeahead): support for grouped results (#1274) #3711
Conversation
@maxokorokov Please review |
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; } |
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).
That is the biggest bottleneck in the PR, but I've no idea how to solve it...
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.
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" .... >
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.
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.
benouat
May 18, 2020
Member
Let me rephrase my concern here:
@Input() context
on the component.
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
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 providingthis
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
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; | ||
} | ||
} |
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
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...
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 { |
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.
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
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...
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:
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? |
This is the blocking issue: angular/angular#14935 |
@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. |
@benouat It seems there is an error on Travis side. Am I right, or there is an error on my side? Please chec. |
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 |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
@gpolychronis what is this last force push about ? |
An attempt to rebuild and check again the tests. It worked. |
|
Done. @maxokorokov , @benouat please review. |
I have seen a lot of progress in the PR |
<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"> |
benouat
Aug 25, 2020
Member
This is not related to this PR, probably just a formatting issue, changes should be discarded.
@@ -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. |
set autocomplete(_autocomplete: string) { | ||
this._autocomplete = _autocomplete; | ||
if (this._inputRef) { | ||
this._inputRef.nativeElement.setAttribute('autocomplete', this._autocomplete); |
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
* 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. |
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 ...
@@ -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. |
</button> | ||
` | ||
}) | ||
export class NgbTypeaheadItem { |
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
/** | ||
* The context for the typeahead content template in case you want to override the default one. | ||
*/ | ||
export interface ContentTemplateContext { |
benouat
Aug 25, 2020
Member
This context class should be somehow using Generics like ContentTemplateContext<T>
where results: T[]
[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"> |
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
</ng-template> | ||
<ng-template [ngTemplateOutlet]="contentTemplate || defaultContentTemplate" | ||
[ngTemplateOutletContext]="{results: results}"></ng-template> |
benouat
Aug 25, 2020
Member
You could use here in addition to results
the $implicit
key. Simply duplicate thing.
{
$implicit: results,
results: results
}
templateUrl: './typeahead-grouping.html', | ||
styles: [`.form-control { width: 300px; }`] | ||
}) | ||
export class NgbdTypeaheadGrouping { |
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...
0f5f98f
to
413c091
Just a note: One typeahead template variable "result" is renamed to "item". This will probably break backwards compatibility. |
Before submitting a pull request, please make sure you have at least performed the following: