phetsims / area-builder

"Area Builder" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/area-builder
GNU General Public License v3.0
1 stars 2 forks source link

fragile code for creating shapes carousel #82

Closed pixelzoom closed 8 years ago

pixelzoom commented 8 years ago

I ran across this while looking for examples to steal for function-builder.

There is a bit of fragile code in AreaBuilderGameView, at line 781:

        if ( creatorNodes.length > 4 ) {
            // Add a scrolling carousel.
            this.shapeCarouselRoot.addChild( new Carousel( creatorNodes, {
              ...
            } ) );
          }
        else {
            // Add a non-scrolling panel
           ...
          }

If the intent is to create a Carousel only if there is >1 page of items, then this implementation works only because 4 happens to be the default value of Carousel options.itemsPerPage.

Highly recommended to change this to:

        var itemsPerPage = 4;
        if ( creatorNodes.length > itemsPerPage ) {
            // Add a scrolling carousel.
            this.shapeCarouselRoot.addChild( new Carousel( creatorNodes, {
                itemsPerPage: itemsPerPage
              ...
            } ) );
          }
        else {
            // Add a non-scrolling panel
           ...
          }

Assigned to @jbphet, will let him decide whether to change this.

jbphet commented 8 years ago

Good observation @pixelzoom - this dates back to when the carousel wasn't a shared component, but even so, it would be fragile code. I did as suggested, though I made ITEMS_PER_PAGE into a constant rather than a variable. I tested the change by varying the value for this constant and re-running the sim, and it worked fine. Closing.