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

fix(VirtualScroll): initialize trackByFn reference #11624

Merged
merged 2 commits into from
May 19, 2017

Conversation

pluscubed
Copy link
Contributor

Short description of what this resolves:

The virtualScroll input setter can be called first, which calls initializes _differ with the default virtualTrackBy function. When the virtualTrackBy input setter is called later, _differ is no longer blank, and so the input virtualTrackBy is not used.

Changes proposed in this pull request:

Do not check whether _differ is undefined in the virtualTrackBy setter.

Ionic Version: 3.x
Fixes: Issue #7531, PR #11492

@@ -528,7 +528,13 @@ export class VirtualScroll implements DoCheck, AfterContentInit, OnDestroy {
}

private _updateDiffer() {
if (isBlank(this._differ) && isPresent(this._records)) {
Copy link
Contributor

@masimplo masimplo May 18, 2017

Choose a reason for hiding this comment

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

Removing isBlank check from the current codebase will have the desired effect. If interested, please updated your pull request to only alter this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

what was the reason to have isBlank(this._differ) in first instance?

Copy link
Contributor

Choose a reason for hiding this comment

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

This was from the original code that initialized this._differ only when setting VirtualScroll property as a guard to not reinitialize the differ again. Now we need to initialize it twice. Once when setting VirtualScroll and once when setting TrackBy. If trackBy is not provided, then differ will be initialized with the default comparer.
When DRYing the code in my pull request #11492 I accidently kept the original code that only accounted for differ being set once.

@manucorporat manucorporat merged commit 892e14f into ionic-team:master May 19, 2017
@naveedahmed1
Copy link

@manucorporat @masimplo can this issue (https://github.com/driftyco/ionic/issues/11788) be a side effect of this change?

@masimplo
Copy link
Contributor

@naveedahmed1 Can't imagine how this can be related. Are you using virtualTrackBy and removing it solves your issue?

@naveedahmed1
Copy link

If I am not wrong, probably this was the only change made to virtual scroll in this release, correct? The issue persists either I add virtualTrackBy or remove it.

@masimplo
Copy link
Contributor

Pretty sure it is not related with virtualTrackBy change then.

@ibnclaudius
Copy link

ibnclaudius commented May 30, 2017

For some reason after the fix for this issue (commit 892e14f) my virtual list no longer updates when changing segments. Below is the relevant part of my code for you understand.

this.historyProvider.getHistory()
      .subscribe((history: Copy[]): Copy[] => this.history = history);

<ion-segment [(ngModel)]="segment" (ionChange)="mergeNewHistory()">...</ion-segment>

<ion-list [virtualScroll]="history" [virtualTrackBy]="virtualTrack">
  <button *virtualItem="let copy" ion-item>...</button>
</ion-list>
virtualTrack(index: number, copy: Copy): Date {
  return copy.date;
}

If I go back to version 3.2.1 it works perfectly! I'm pretty sure it's related to this because it was the only change that occurred in Virtual Scroll.

@masimplo
Copy link
Contributor

@ibnclaudius VirtualTrackBy was not working up to now, so it is possible your function was not called at all previously. Please check if the function is invoked when you run it against 3.2.1 at all.

@ibnclaudius
Copy link

@masimplo You're right! The function was not being called! The only issue now is that virtualscroll is not rebuilding the list when I set it empty.

@masimplo
Copy link
Contributor

Was pretty sure that fixing VirtualTrackBy would give a lot of people head scratching moments as people had written all sort of things in trackByFn that had no effect until now and it might not do what they expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants