Skip to content

support an optional trackBy function in FlatTreeControl #18708

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

Merged
merged 5 commits into from Apr 20, 2020
Merged

support an optional trackBy function in FlatTreeControl #18708

merged 5 commits into from Apr 20, 2020

Conversation

ghost
Copy link

@ghost ghost commented Mar 4, 2020

This is similar to trackBy in *ngFor and is critical for those of us who
use the redux pattern (i.e. NGRX) since we frequently replace our node references with
modified copies from the Store (as opposed to changing the state within the node objects
themselves)

This is similar to trackBy in *ngFor and is critical for those of us who
use the redux pattern (i.e. NGRX) since we frequently replace our node references with
modified copies from the Store (as opposed to changing the state within the node objects
themselves)
@ghost ghost requested review from andrewseguin and jelbourn as code owners March 4, 2020 16:50
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 4, 2020
@jelbourn jelbourn added the G This is is related to a Google internal issue label Mar 5, 2020
@andrewseguin
Copy link
Contributor

Two comments:

  • The expansionModel cannot be unknown
  • Currently this would be a breaking change and you'd have to wait until the next major to get it out

To resolve this, we can add a new type K that will represent the type returned by the trackBy function. We can default it to T so that it remains backwards compatible.

You can take the code from this commit and apply it to this PR and verify that it fits your use case: andrewseguin@d0c1089

@ghost
Copy link
Author

ghost commented Mar 25, 2020

Hi! Thank you for the suggested changes - I've applied them, but I'm not sure what these failing CircleCI tests are

@jelbourn
Copy link
Member

jelbourn commented Mar 25, 2020

There's a build failure

ERROR: /home/circleci/ng/src/cdk/tree/BUILD.bazel:28:1: Compiling TypeScript (devmode) //src/cdk/tree:unit_test_sources failed (Exit 1)
src/cdk/tree/control/flat-tree-control.spec.ts:147:5 - error TS2322: Type '(node: TestData) => string' is not assignable to type '(dataNode: TestData) => TestData'.
  Type 'string' is not assignable to type 'TestData'.

147     treeControl.trackBy = (node: TestData) => `${node.a} ${node.b} ${node.c}`;

@ghost
Copy link
Author

ghost commented Apr 14, 2020

Hello! Fixed up the failing tests, I'm not sure what that last check is though.

@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Apr 14, 2020
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@andrewseguin andrewseguin added lgtm action: merge The PR is ready for merge by the caretaker labels Apr 14, 2020
@jelbourn jelbourn added the P2 The issue is important to a large percentage of users, with a workaround label Apr 20, 2020
@jelbourn jelbourn merged commit 04d1588 into angular:master Apr 20, 2020
soro-google pushed a commit to soro-google/components that referenced this pull request Apr 24, 2020
…18708)

This is similar to trackBy in *ngFor and is important for those who
use the redux pattern (i.e. NGRX) since node references are frequently replaced with modifies instances from the Store (as opposed to changing the state within the node objects
themselves)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement G This is is related to a Google internal issue P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants