Next/previous buttons are hidden for 1-page Carousel

[pixelzoom]

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

In sun demo Components screen, the CarouselComboBox for choosing a demo now looks like this. There are blank spaces at the top and bottom of the listbox (which is a Carousel):

I think I understand what's going on. This is a 1-page carousel, and the next/previous buttons are probably being hidden. But this looks odd to me. Is this the new default behavior for 1-page Carousel? If so, was it discussed with designers, and is it still open for discussion? Would it be better to close up the empty spaces -- that should certainy be doable with new scenery layout.

I'd be fine with opting in to this behavior, but I don't think it should be the default.

[pixelzoom]

Looks like this is not only the default behavior, but is the only behavior. In Carousel.ts:

    const buttonsVisibleProperty = new DerivedProperty( [ this.numberOfPagesProperty ], numberOfPages => {
      // always show the buttons if there is more than one page, and always hide the buttons if there is only one page
      return numberOfPages > 1;
    } );

What is the reasoning for showing blank spaces where the buttons were?

[pixelzoom]

I see the specific case that I reported mentioned is mentioned in the "Major features" of https://github.com/phetsims/sun/issues/814#issuecomment-1379727179:

• Removed the option to "hide carousel buttons when they are disabled" because it did not have a consistent look and feel across sims, and because where it was used, it felt unbalanced/asymmetrical. Note that this looks odd for carousel combo box that has a small number of items. Our understanding was that is OK--this is not yet a production component. That case looks like this:

I don't understand the rationalization that "Our understanding was that is OK--this is not yet a production component". This is not behavior of ComboBoxCarousel, it's behavior of Carousel. So if I remove enough items from a Carousel to reduce it to 1 page, I'm going to see these holes where the buttons were -- and now with no way to opt out. Or do I not understand something?

And was this cleared with designers?

[pixelzoom]

I confirmed that this issue is not specific to ComboBoxCarousel, and affects all Carousels/

I ran CCK:DC in Studio, and set visibleProperty to false for all but 5 of ithe items in circuitElementToolbox.carousel. That gives me a 1-page Carousel that looks like this:

This looks buggy to me, not like a "feature". It also seems odd that the PageControl continues to be shown, but that's probably a sim-specific details that isn't being handled.

If I continue to make items invisibile, the Carousel gets shorter:

So I do not see an advantage to leaving blank space when the buttons are invisible.

[pixelzoom]

Affecting Number Play suite, see https://github.com/phetsims/number-suite-common/issues/48#issuecomment-1419390817:

[pixelzoom]

@samreid and I discussed. It looks promising to consider button visibility in computation of backgroundSizeProperty in Carousel.ts. I'll have a look.

    // Background size - includes the buttons
    const backgroundSizeProperty = new DerivedProperty( [ windowSizeProperty ], windowSize => {
      return new Dimension2(
        windowSize.width + ( isHorizontal ? nextButton.width + previousButton.width : 0 ),
        windowSize.height + ( isHorizontal ? 0 : nextButton.height + previousButton.height )
      );
    } );

[pixelzoom]

In https://github.com/phetsims/sun/commit/80af852c912983fa86a03d25e92b5bd0ab96bb69, @samreid and I changed the derivation of backgroundSizeProperty. The button only contribute when they are visible. This had the desired result of removing the holes. We tested in sun demo and (using Studio to add/remove items) in CCK:DC.

In https://github.com/phetsims/sun/commit/d833b6c3a6fdcc879eda8b699a09b570e943b195, I cleaned up the derivation of backgroundSizeProperty.

@samreid want to take a look? Feel free to close.

[samreid]

The commits look correct to me, thanks! Closing.