search

phetsims / sun

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

Review Carousel accessibility modifiers #831

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Related to #814 and https://github.com/phetsims/sun/issues/829 ...

Do you really want getAlignBoxForItem and scrollToItemVisibleIndex to be public? They seem specific to Carousel internals, and are in fact only used inside Carousel.

samreid commented 1 year ago

Yes, they are currently only used in Carousel. But getAlignBoxForItem is designed to allow clients to remove something from the carousel without collapsing the empty space. 

  /**
   * Can control the visibility of this AlignBox to determine whether the space inside the carousel is maintained
   */
  public getAlignBoxForItem( item: CarouselItem ): AlignBox {

I'll make scrollToItemVisibleIndex private (for now?)

samreid commented 1 year ago

Fixed, ready for review.

pixelzoom commented 1 year ago

.... But getAlignBoxForItem is designed to allow clients to remove something from the carousel without collapsing the empty space.

It's unfortunate (and imo unnecessary) that you're exposing the internals of Carousel to support that.

samreid commented 1 year ago

What if we change the return type to Node and give it a more general name?

pixelzoom commented 1 year ago

@samreid and I reviewed on Zoom. We made getAlignBoxForItem, because it's not actually useful for leaving holes.

Closing.