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

Virtual Scroll fix: this.approxItemHeight is undefined when update(true) is called first #8350

Merged

Conversation

ckaeslin
Copy link
Contributor

Short description of what this resolves:

this.approxItemHeight is undefined when update(true) is called first
see #8311

Changes proposed in this pull request:

  • define this.approxItemHeight to default 40px before do this.update(true)

Ionic Version: 2.1.0

Fixes: #8311

@brandyscarney
Copy link
Member

Hello @ckaeslin, thanks for the PR! How can I reproduce this? Is it random or is there something I can do to always get an error? :)

@ckaeslin
Copy link
Contributor Author

ckaeslin commented Oct 12, 2016

This one is pretty easy to reproduce, use the lates Ionic2 RC0 and create a virtual-scroll (without approxItemHeight="40px").

<ion-list [virtualScroll]="items">
    <ion-list-header>Follow us on Twitter</ion-list-header>
    <ion-item *virtualItem="let item">
      <ion-icon name="ionic" item-left></ion-icon>
      {{item}}
    </ion-item>
</ion-list>

Or checkout this repo https://github.com/jeromeXoo/ionic2.rc0.Virtual-Scroll-Not-Working and remove approxItemHeight="40px" here https://github.com/jeromeXoo/ionic2.rc0.Virtual-Scroll-Not-Working/blob/master/src/pages/contact/contact.html#L10

you will just get a blank page (and an error message: TypeError: Cannot read property 'indexOf' of undefined)

@ckaeslin
Copy link
Contributor Author

ckaeslin commented Oct 12, 2016

Documentation says approxItemHeight is default set to 40px: http://ionicframework.com/docs/v2/api/components/virtual-scroll/VirtualScroll/#input-properties
But this default is not working with the current code.

@brandyscarney
Copy link
Member

Interestingly enough, this error goes away if you move the data out of the constructor into the ionViewDidEnter, like so:

export class ContactPage {

  public items: Array<any> = [];

  constructor(public navCtrl: NavController) {

  }

  ionViewDidEnter() {
    for(let i = 0; i < 100; i++)
    {
      this.items.push({id: i,
         name:'name' +i});
    }
  }
}

While we don't recommend loading data in the constructor of your component, throwing an error isn't a good alternative so I'll look into your PR. Thanks again!

@pacMakaveli
Copy link

pacMakaveli commented Oct 13, 2016

@brandyscarney , sorry, it still happens.

  ionViewDidEnter() {
    this.showLoading('Retrieving Client details..');

    this.clientService.get(this.clientId).then((clientData) => {
      this.clientResponse = clientData;

      this.loading.dismiss().then(() => {
        this.buildings = clientData.buildings;
        this.stores    = clientData.stores;
        this.initClient();
      });
    }, (error) => {
      this.loading.dismiss().then(() => {
        console.log(error);
      });
    });
  }
  <ion-list [virtualScroll]="buildings">
    <!-- <building-detail *virtualItem="let building" [building]="building"></building-detail> -->
    <button *virtualItem="let building" ion-item (click)="goToBuilding(building.id)" class="building-detail">
      <h2 class="building-detail--name">{{ building.name }}</h2>
      <p class="building-detail--address">{{ building.address }}</p>
    </button>
  </ion-list>

@ckaeslin
Copy link
Contributor Author

The problem is that https://github.com/driftyco/ionic/blob/master/src/components/virtual-scroll/virtual-scroll.ts#L346 goes to https://github.com/driftyco/ionic/blob/master/src/components/virtual-scroll/virtual-util.ts#L569, where approxItemHeight is not defined, because it's currently done after this.update(true);

@brandyscarney brandyscarney merged commit b16228b into ionic-team:master Oct 13, 2016
@brandyscarney
Copy link
Member

Look good! Thanks 👍

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.

3 participants