Review changes to expression exchange regarding the carousel

[samreid]

Related to https://github.com/phetsims/sun/issues/814 and #159, @jonathanolson said:

Hey! @jbphet noticed the expression-exchange carousel is resizing on the 2nd screen when "All Coefficients" checkbox is toggled.

It looks like the content items are changing sizes. We'll probably want a good way to support minimum sizes for cells (to pass to the align boxes). Any thoughts on how to support this (should the sim just be wrapping it in things that don't change size?)

I replied:

I'll take a look at the expression-exchange carousel behavior. I see in master it does not resize. I reproduced the problem in expression exchange where carousel items change size and the carousel resizes. I agree it doesn't look so good.

The worst case looks like this:

Any thoughts on how to support this (should the sim just be wrapping it in things that don't change size?)

I followed that strategy in this patch:

```diff Subject: [PATCH] Fix default group, see https://github.com/phetsims/circuit-construction-kit-common/issues/937 --- Index: js/common/view/CoinTermCreatorBoxFactory.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CoinTermCreatorBoxFactory.js b/js/common/view/CoinTermCreatorBoxFactory.js --- a/js/common/view/CoinTermCreatorBoxFactory.js (revision b679d04bd3d967c11c7e275126ccf71df04b3f79) +++ b/js/common/view/CoinTermCreatorBoxFactory.js (date 1675477369116) @@ -153,7 +153,7 @@ options = merge( { itemsPerCarouselPage: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 4 : 3, - itemSpacing: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 35 : 40 + itemSpacing: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 5 : 10 }, options ); // create the list of creator nodes from the descriptor list @@ -180,7 +180,7 @@ */ createGameScreenCreatorBox( challengeDescriptor, model, view, options ) { options = merge( { - itemSpacing: 30, + itemSpacing: 5, align: 'top' }, options ); Index: js/common/view/CoinTermCreatorBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CoinTermCreatorBox.js b/js/common/view/CoinTermCreatorBox.js --- a/js/common/view/CoinTermCreatorBox.js (revision b679d04bd3d967c11c7e275126ccf71df04b3f79) +++ b/js/common/view/CoinTermCreatorBox.js (date 1675477961308) @@ -10,8 +10,10 @@ */ import merge from '../../../../phet-core/js/merge.js'; -import { Node } from '../../../../scenery/js/imports.js'; +import { Node, Rectangle } from '../../../../scenery/js/imports.js'; import Carousel from '../../../../sun/js/Carousel.js'; +import { Shape } from '../../../../kite/js/imports.js'; +import Vector2 from '../../../../dot/js/Vector2.js'; import expressionExchange from '../../expressionExchange.js'; class CoinTermCreatorBox extends Node { @@ -26,7 +28,7 @@ options = merge( { itemsPerCarouselPage: 3, - itemSpacing: 20, // empirically determined to work for most cases in this sim + itemSpacing: 5, // empirically determined to work for most cases in this sim cornerRadius: 4, align: 'center' }, options ); @@ -40,11 +42,30 @@ // @private {Node} this.coinTermCreatorBox = new Carousel( creatorNodes.map( element => { - return { createNode: tandem => element }; + return { + createNode: tandem => { + + // Pick bounds that fit every single thing in the sim. + const H = 90; + const W = 140; + + // Could be a Node, but this helps with debugging if you want to see the bounds. + const panel = new Rectangle( 0, 0, W, H, { + fill: 'transparent', + children: [ element ] + } ); + element.center = new Vector2( W / 2, H / 2 ); + + // Prevent resizing when elements bounds want to go outside the rectangle + panel.clipArea = Shape.rect( 0, 0, W, H ); + + return panel; + } + }; } ), { itemsPerPage: options.itemsPerCarouselPage, - spacing: options.itemSpacing, - margin: 14, + spacing: 5, + margin: 5, cornerRadius: options.cornerRadius } ); this.addChild( this.coinTermCreatorBox ); ```

It looks reasonable enough in every circumstance I tried, but it also seems kind of brittle if some coins or terms change size and this doesn't accommodate. Luckily I didn't see any translateable strings in the coins or terms. If this is the last part, we may proceed and request help from @jbphet if pixel polishing is necessary. Everything else seems to be working, so I'm going to go for it. I'll open an expression exchange issue for @jbphet to take a closer look once merged back to master.

Summarizing the problem and my main concerns:

• Carousel now automatically reshapes/resizes when its items resize. However, in Expression Exchange, we want the items to change size without readjusting the carousel. But we don't know the max size of the items at the outset--it depends on several different settings.
• The committed solution is to put a bounding box around the content, and use a clipping region so nothing leaks out. This should be reviewed visually to make sure there is no corner case that is buggy. The main problem with the committed solution is that it hard codes a "max size" for the items, which does not dynamically update. Could be brittle.
• I also saw a lot of fine tuning variables adjusting margins and spacing for all the different circumstances. I did not make an effort to pixel-match or equally tune everything. The goal was to get it working and looking reasonable.

I figure we can do any necessary fine-tuning when we revisit this sim for publication, but I want to leave it in a reasonable state in the meantime, so I wanted to reach out to @jbphet for assistance when there's time.

[jbphet]

I tested the behavior in the master version of the sim and it seems fine, but I agree that the fix is a bit brittle (and, admittedly, the original code was too). I for one would not object to the carousel changing size as the things inside of it change, but that's a design decision, and it doesn't seem like it is worth spending time on at the moment. I'll unassign this issue and will mark it as blocking sim publication, and we will revisit this the next time it is published from master.

FWIW, another possible solution might be to use a transparent container node of a fixed size to hold the items in the carousel and limit the items to fit within it.