phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Carousel `scrollToItemVisibleIndex` does not compensate for visibility #829

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Related to #814 ...

Items can be made invisible via PhET-iO. As I understand it, that was the main motivation for #814. And that affects programmatic scrolling to those items, which currently looks a little buggy.

Relevant bits of Carousel.ts:

  public scrollToItemVisibleIndex( itemIndex: number ): void {
    this.pageNumberProperty.set( this.itemVisibleIndexToPageNumber( itemIndex ) );
  }

  public scrollToItem( item: CarouselItem ): void {
    // If the layout is dynamic, then only account for the visible items
    const alignBoxIndex = this.visibleAlignBoxesProperty.value.indexOf( this.getAlignBoxForItem( item ) );

    assert && assert( alignBoxIndex >= 0, 'item not present or visible' );

    this.scrollToItemVisibleIndex( alignBoxIndex );
  }

  private itemVisibleIndexToPageNumber( itemIndex: number ): number {
    assert && assert( itemIndex >= 0 && itemIndex < this.items.length, `itemIndex out of range: ${itemIndex}` );
    return Math.floor( itemIndex / this.itemsPerPage );
  }

It looks like scrollToItem compensates for item visibilty. But if I call scrollToItemVisibleIndex, I'm going to bet that it does not behave correctly if items have been made invisible via PhET-iO. (I'm doing this via inspection, did not write a test case.)

The easist fix would be to make scrollToItemVisibleIndex private; it's only used inside of Carousel. Otherwise scrollToItemVisibleIndex should probably be responsible for adjusting index (instead of scrollToItem).

scrollToItemVisibleIndex could also use better doc or a better name. It's not totally clear on first reading what a "visible index" is. I guessed what it is because I'm aware of the visibility issue.

samreid commented 1 year ago

This seems related to https://github.com/phetsims/sun/issues/830

pixelzoom commented 1 year ago

@samreid and I discussed on Zoom. We made scrollToItemVisibleIndex private, and changed its param to itemVisibleIndex: boolean to clarify. Closing.