search

phetsims / sun

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

Deprecate legacy support in GroupItemOptions #794

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

GroupItemOptions is used to specify the "items" in various "groups" -- AquaRadioButtonGroup, VerticalCheckboxGroup, etc.

It currently supports 2 mutually-exclusive ways of specifying options:

// the legacy way
{
  node: Node,
  tandem?: Tandem
}

// the new way
{
  createNode: ( tandem: Tandem ) => Node,
  tandemName: string
}

Supporting both complicates code, and can result in problems (e.g. https://github.com/phetsims/sun/issues/793#event-7472017982). So we'd eventually like to deprecate and remove support for "the legacy way".

pixelzoom commented 1 year ago

VerticalCheckboxGroupItem and AquaRadioButtonGroupItem are currently the only places where GroupItemOptions is used.

There are 12 occurrences of import VerticalCheckboxGroup, 14 occurrences of import AquaRadioButtonGroup.

samreid commented 1 year ago

I'm going to remove the legacy way and port all sims to use the new way.

samreid commented 1 year ago

After changes in https://github.com/phetsims/sun/issues/794, you can no longer specify node/tandem to AquaRadioButtonGroup and VerticalCheckboxGroup. Instead, please use createNode and tandemName.

I've left a few ts-ignore TODOs in the code for this issue, for after a few CT cycles.

pixelzoom commented 1 year ago

Slack#continuous-testing:

Kathryn Woessner A number of sims are failing with an error that contains e.createNode is not a function. Any ideas?

Sam Reid That’s me, I’ll take a look, thanks

samreid commented 1 year ago

My earlier test unfortunately only ran aqua for the PhET-iO Sims. I reran aqua for all sims and things are looking much better now. Thank you for bringing this to my attention. I will continue to watch CT over the next few columns in case things were missed by fuzzing here.

samreid commented 1 year ago

This seems fixed, closing.