phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

AquaRadioButtonGroup and VerticalAquaRadioButtonGroup should be parametric types #728

Closed samreid closed 2 years ago

samreid commented 2 years ago

While working on https://github.com/phetsims/circuit-construction-kit-common/issues/606, I discovered that AquaRadioButtonGroup and VerticalAquaRadioButtonGroup could handle types better even in JavaScript if they use the `/ @template T / annotation like we do in Property. I'll add this.

samreid commented 2 years ago

Fixed and working in CCK. This introduced compiler errors in Geometric Optics, so I opened https://github.com/phetsims/geometric-optics/issues/265 and addressed it. Still, this issue was straightforward and seems it can be closed.

pixelzoom commented 2 years ago

Reopening.

The type expression for items is incorrect, it's referring to Node built-in, not scenery.Node. And the use of as unknown as Node in geometric-optics is also incorrect, referring to Node built-in.

This problem is more generally described in https://github.com/phetsims/chipper/issues/1142. The correct type expression in AquaRadioButton, AquaRadioButtonGroup and its subtypes scenery.Node, i.e.:

- @param { {node:Node,value:T,tandemName?:string,labelContent?:string}[]} items
+ @param { {node:scenery.Node,value:T,tandemName?:string,labelContent?:string}[]} items
pixelzoom commented 2 years ago

You've also introduced this problem into CCKCOptionsDialogContent.js.

For example:

node: new Text( circuitConstructionKitCommonStrings.ieee, textOptions ) as unknown as Node,

... does not refer to scenery.Node, it refers to built-in Node.

pixelzoom commented 2 years ago

Summary of the above commits:

Over to @samreid to review and close.

EDIT: If there are any other places where you added incorrect as unknown as Node, I didn't find them, and did not address them.

pixelzoom commented 2 years ago

It would also be good to parameterize RectangularRadioButtonGroup. I now have this odd difference in geometric-optics, where only Aqua radio buttons are parameterized:

class FocalLengthControlRadioButtonGroup extends VerticalAquaRadioButtonGroup<FocalLengthControlEnum> {

class RaysRadioButtonGroup extends VerticalAquaRadioButtonGroup<RaysModeEnum> {

class OpticShapeRadioButtonGroup extends RectangularRadioButtonGroup {
samreid commented 2 years ago

I made RectangularRadioButtonGroup parametric in the commit, can you please review?

pixelzoom commented 2 years ago

RectangularRadioButtonGroup land its usages in .ts look good to me.

samreid commented 2 years ago

The changes in https://github.com/phetsims/sun/issues/728#issuecomment-964471758 also look good, so I think this issue can be closed. Please reopen this or a new issue if there's more to do.