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
fix(list): allow setting a custom trackBy function for the underlying ngFor directive #435
fix(list): allow setting a custom trackBy function for the underlying ngFor directive #435
Conversation
Deploy preview for patternfly-ng ready! Built with commit 57cdca0 |
0b187e5
to
6ec1ac8
Compare
6ec1ac8
to
36895b7
Compare
@@ -162,6 +167,10 @@ export class ListComponent extends ListBase implements DoCheck, OnInit { | |||
return result + '-' + suffix + index; | |||
} | |||
|
|||
protected getTrackBy(index: number, item: any): string { | |||
return `track-${index}`; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function appears to be unused.
@@ -6,7 +6,7 @@ import { | |||
Input, | |||
OnInit, | |||
Output, | |||
TemplateRef, | |||
TemplateRef, TrackByFunction, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
own line?
showCheckbox: false, | ||
useExpandingRows: false | ||
} as ListConfig; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much of this config is duplicated, top of the spec. Can we reuse any of that?
getTrackBy(index: number, item: any): any { | ||
return index; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a new trackBy example created instead of altering the basic one. Want to ensure our basic use-case continues to work for existing applications
getTrackBy(index: number, item: any): any { | ||
return index; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create one, new example for "trackBy". That will help ensure existing applications continue to work as is.
Tested this against Syndesis UI and it works like a charm - nice enhancement |
Thanks for the review @dlabrecq! I have made some changes to the code, restored the examples and added a new one. |
855cf99
to
00e7e3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for creating a new example. Just a couple nits left
<include-content src="src/app/list/basic-list/example/list-basic-example.component.html"></include-content> | ||
</tab> | ||
<tab heading="typescript"> | ||
<include-content src="src/app/list/basic-list/example/list-basic-example.component.ts"></include-content> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to show the correct example, these two links should use list-polling-example.component.ts
instead of list-basic-example.component.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah! Good catch, thanks!
useExpandItems: true | ||
} as ListConfig; | ||
|
||
this.updateItemsInterval = setInterval(() => this.updateItems(), 2500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm seeing an error in my IDE, which states "Type 'Timer' is not assignable to type 'number'. However, using this seems to work:
updateItemsInterval: number = <any>setInterval(() => {});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it too but assumed it was something specific to my IDE (WebStorm). I'll update this to use any
then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the contribution!
It is now possible to set a custom trackBy function to be used in the underlying ngFor directive, which by default tracks changes in the items by identity.
This is useful in scenarios where the items passed to the component are periodically polled, and components with state (eg. a
pfng-action
component) are used in the item's template.This should help fixing syndesisio/syndesis/issues/3121 where we have exactly this problem.