Issues with SecondLocaleSelectorCarousel

[pixelzoom]

Notes while working on https://github.com/phetsims/number-suite-common/issues/36 ...

There are a couple of significant problems with SecondLocaleSelectorCarousel:

(1) It's name. The titme of the control is "Second Language", not "Second Locale". This will become a problem when this code is eventually instrumented for PhET-iO, because the convention is to name things based on the name that you see in the UI.

(2) For some reason, SecondLocaleSelectorCarousel chunks the languages into a set of GridBoxes, each with 10 rows. It then displays 1 Grid at a time. This is totally unnecessary, duplicating the responsibility of Carousel.

     // A prototype where we show all languages in grid managed by a Carousel so that there aren't too many items
    // displayed at one time
    const chunkedLocaleItems = _.chunk( createInteractiveLocales(), 10 );
    const carouselItems = chunkedLocaleItems.map( localeItem => {
      return new GridBox( {
        xMargin: 5,
        yMargin: 3,
        xAlign: 'center',
        autoRows: 10,
        children: [ ...localeItem ],
        resize: false
      } );
    } );

[pixelzoom]

In the above commit, SecondLocaleSelectorCarousel was absorbed into SecondLanguageControl. It's greatly simplified, and no longer requires a subclass, see below. @chrisklus please review, close if OK.

    // Carousel for choosing the second language.
    const carouselItems = localeProperty.validValues!.map(
      locale => new LanguageSelectionNode( secondLocaleProperty, locale ) );
    const secondLanguageCarousel = new Carousel( carouselItems, {
      visibleProperty: showSecondLocaleProperty,
      itemsPerPage: 10,
      spacing: 6,
      margin: 5,
      orientation: 'vertical'
    } );

[pixelzoom]

Also note the problem reported and addressed in:

• https://github.com/phetsims/number-suite-common/issues/38

[chrisklus]

Thanks @pixelzoom, that's much simpler. I like that the accordion box stays full-height when there are few language options too. Closing.

[pixelzoom]

Reopening:

@chrisklus said:

... I like that the accordion box stays full-height when there are few language options too.

I finally undersand what you meant by this. When I ran with locales=es,en, I saw this:

That looks really wrong, and I'm sort of surprised that Carousel behaves that way. And I see no good reason to a single-page Carousel with lots of empty cells.

So in the above commit, I fixed the options passed to Carousel:

- itemsPerPage: 10,
+ itemsPerPage: Math.min( 10, carouselItems.length ),

That results in this for locales=en,es,fr:

And this for locales=*:

@chrisklus please review, close (again) if OK.