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

docs(common): rewrite docs for NgForOf#ngForTrackBy #42329

Closed
wants to merge 3 commits into from

Conversation

IgorMinar
Copy link
Contributor

Clarify the prupose of the tracking function and document how to create a good one.

Fixes #40461

@google-cla google-cla bot added the cla: yes label May 25, 2021
@IgorMinar IgorMinar requested a review from zarend May 25, 2021 20:51
@pullapprove pullapprove bot requested a review from atscott May 25, 2021 20:51
@IgorMinar IgorMinar removed the request for review from zarend May 25, 2021 20:52
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461
@IgorMinar IgorMinar added action: review The PR is still awaiting reviews from at least one requested reviewer comp: docs target: patch This PR is targeted for the next patch release labels May 25, 2021
@ngbot ngbot bot modified the milestone: Backlog May 25, 2021
@IgorMinar IgorMinar requested a review from TeriGlover May 25, 2021 20:58
@IgorMinar
Copy link
Contributor Author

@TeriGlover can you please take a look at this one as well? thank you!

@mary-poppins
Copy link

You can preview f2716b0 at https://pr42329-f2716b0.ngbuilds.io/.

@mary-poppins
Copy link

You can preview 4197b82 at https://pr42329-4197b82.ngbuilds.io/.

Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

Hey @IgorMinar, do you think you could take on #35896 (in another PR) as well since it's related? The issue there is that the insertion order of VE just happened to work with ngFor in the select (#35896 (comment)). All the items in the select array are new objects because of the map function so Angular needs to be able to identify which ones are pre-existing via trackBy.

Also, based on that issue maybe it's worth adding a special note in your description to call out the importance of trackBy with select elements and their options.

@IgorMinar
Copy link
Contributor Author

I'll take a look @atscott. Thanks for the review.

Copy link
Contributor

@TeriGlover TeriGlover left a comment

Choose a reason for hiding this comment

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

Editing review complete; left several comments.

packages/common/src/directives/ng_for_of.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_for_of.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_for_of.ts Outdated Show resolved Hide resolved
packages/common/src/directives/ng_for_of.ts Outdated Show resolved Hide resolved
@TeriGlover
Copy link
Contributor

TeriGlover commented May 25, 2021 via email

@TeriGlover
Copy link
Contributor

TeriGlover commented May 25, 2021 via email

@IgorMinar
Copy link
Contributor Author

thanks for the review @TeriGlover, I'll work on the additions that Andrew brought up in a new PR and will ask you for a review.

@IgorMinar IgorMinar added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels May 25, 2021
@mary-poppins
Copy link

You can preview 4471331 at https://pr42329-4471331.ngbuilds.io/.

@givo
Copy link

givo commented May 26, 2021

Hi guys,

This is much better now. Docs were missing the reorder part. Now it'a much more clearer.

Many thanks!

zarend pushed a commit that referenced this pull request May 26, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes #40461

PR Close #42329
@zarend zarend closed this in 14abc68 May 26, 2021
umairhm pushed a commit to umairhm/angular that referenced this pull request May 28, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461

PR Close angular#42329
iRealNirmal pushed a commit to iRealNirmal/angular that referenced this pull request Jun 4, 2021
Clarify the prupose of the tracking function and document how to create a good one.

Fixes angular#40461

PR Close angular#42329
@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 Jun 26, 2021
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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve documentation of behavior for ngFor's trackBy
6 participants