PhET-iO Design for Carousel

[arouinfar]

Related to https://github.com/phetsims/circuit-construction-kit-common/issues/513

Here's an example of what carousel currently looks like in CCK: DC:

Questions:

• Is it normal/expected for the carousel contents to be siblings of the carousel as in the screenshot above? In balancing-act, the contents are children (e.g. balancingAct.balanceLabScreen.view.massCarousel.brickStackCreatorNode1). However, I'm not sure, since that sim is ancient PhET-iO and possibly a non-standard carousel to boot.
• Similarly, is there a reason why the pageControl is not a child of the carousel? You can hide the carousel, but leave behind the dots which seems very weird to me.

Long-term wishlist:

• https://github.com/phetsims/circuit-construction-kit-common/issues/630

Desired changes:

• [x] carousel.visibleProperty phetioFeatured: true by default
• [x] carousel.pageNumberProperty phetioFeatured: true by default
• [x] carousel.nextButton.enabledProperty and carousel.previousButton.enabledProperty should be phetioReadOnly: true (otherwise causes problems like https://github.com/phetsims/circuit-construction-kit-common/issues/844 and https://github.com/phetsims/circuit-construction-kit-common/issues/848)
• [x] If possible, make carousel.nextButton.enabledProperty and carousel.previousButton.enabledProperty phetioFeatured: false by default. I know they inherit the phetioFeatured behavior from buttons, but in this case, it doesn't make sense to feature something that is read-only. If this can't be done within carousel itself, designers can un-feature it in the overrides instead.
• [ ] If possible, make pageControl a child of carousel.
• [x] pageControl.visibleProperty phetioFeatured: true by default
• [x] If possible, make the visibleProperty of each Node within the carousel phetioFeatured: true by default. (If this is a sim-specific thing, designers can handle it in the overrides.)

Assigning to @zepumph and @samreid. Happy to discuss further on a call or at PhET-iO meeting if these requests are unclear/unreasonable.

[zepumph]

@samreid, can you take the lead on this since you are working on this with CCK? Let me know if you would like assistance.

[samreid]

I completed all the requests above except for:

If possible, make pageControl a child of carousel.

There is difficulty in this request, since PageControl can be used to control any item and Carousel doesn't define how PageControl layout works. Here are some options:

• Leave it as it is and see if it is good enough. The major downsides of the current implementation are (a) pageControl not nested under carousel, and making the carousel invisible doesn't hide the page controls
• Leave the architecture as it is, but nest the pageControl tandem under the carousel tandem (without changing the scene graph). This would fix the tandem tree, but would still leave the page control visible if the user hides the carousel.
• Wire up the visibility of the page control to the visibility of the carousel. This would be easy if it is "always matched" and more complex if we want to make it so the page control can be hidden while the carousel is still showing
• Both of the above bullet points could be implemented
• Reimplement carousel so that it creates and is aware of its nested PageControl, and manages the layout, tandem, visibility, etc. This would be more work but maybe is best for the long run. Would decrease flexibility but make it simpler for other sims to match the same pattern.

My recommendation is that the current implementation is very flexible. Showing the page control outside of the carousel tandem seems reasonable to me, since they are side by side. We could introduce a carouselContainer node that contains both if the user needs one shutoff valve. But also, I don't think it's too much to ask a client to hide the carousel and the page controls if that is the desired behavior.

Ready for review and discussion.

[arouinfar]

@samreid I reviewed in CCK:DC, and the carousel looks great! Thanks for providing more context around PageControl. I don't think we should force it anywhere or wire it up in an unorthodox way.

We could introduce a carouselContainer node that contains both if the user needs one shutoff valve.

Currently carousel and pageControl are both children of circuitElementToolbox. Can we create circuitElementToolbox.visibleProperty?

[samreid]

In the commit, I instrumented circuitElementToolbox and additionally featured its visibleProperty. Let me know if that last part is not desired or if it should be in the overrides. Otherwise, ready for review and please close if all is well.

[arouinfar]

Thanks @samreid. I think circuitElementToolbox.visibleProperty looks good, and it should certainly be phetioFeatured: true. It seems appropriate to handle in the overrides since it is sim-specific, but I'll leave that up to you.

[samreid]

Sounds good, thanks! I also noted that the phetioFeatured: true is advantageous in the code since circuit-construction-kit-common is used by several sims. Closing.