phetsims / sun

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

Standardize constructor signature for UI components that control a Property. #769

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

For UI components that control a Property, I'd like to standardize constructors so that the Property is the first parameter. Some examples:

// Checkbox
- public constructor( content: Node, property: Property<boolean>, providedOptions?: CheckboxOptions )
+ public constructor( property: Property<boolean>, content: Node, providedOptions?: CheckboxOptions )

// ComboBox
- public constructor( items: ComboBoxItem<T>[], property: Property<T>, listParent: Node, providedOptions?: ComboBoxOptions )
+ public constructor( property: Property<T>, items: ComboBoxItem<T>[], listParent: Node, providedOptions?: ComboBoxOptions )

This would be more consistent with other UI components:

// AquaRadioButton
public constructor( property: IProperty<T>, value: T, labelNode: Node, providedOptions?: AquaRadioButtonOptions )

// AquaRadioButtonGroup
public constructor( property: Property<T>, items: AquaRadioButtonGroupItem<T>[], providedOptions?: AquaRadioButtonGroupOptions )

// RectangularRadioButton
  public constructor( property: IProperty<T>, value: T, providedOptions?: RectangularRadioButtonOptions ) {

// RectangularRadioButtonGroup
public constructor( property: Property<T>, items: RectangularRadioButtonItem<T>[], providedOptions?: RectangularRadioButtonGroupOptions )

More importantly, I think it would make usage sites read better, expecially where instantiaton of "content" Nodes are inlined. For example, in build-a-nucleus DecayScreenView:

// This is what we currently have.
    const showElectronCloudCheckbox = new Checkbox(
      new HBox( {
        children: [
          new Text( buildANucleusStrings.electronCloud, { font: LABEL_FONT, maxWidth: 210 } ),

          // electron cloud icon
          new Circle( {
            radius: 18,
            fill: new RadialGradient( 0, 0, 0, 0, 0, 18 )
              .addColorStop( 0, 'rgba( 0, 0, 255, 200 )' )
              .addColorStop( 0.9, 'rgba( 0, 0, 255, 0 )' )
          } )
        ],
        spacing: 5
      } ),
      this.showElectronCloudBooleanProperty
    );

// Having the Property as the first argument reads better.
    const showElectronCloudCheckbox = new Checkbox(
      this.showElectronCloudBooleanProperty
      new HBox( {
        children: [
          new Text( buildANucleusStrings.electronCloud, { font: LABEL_FONT, maxWidth: 210 } ),

          // electron cloud icon
          new Circle( {
            radius: 18,
            fill: new RadialGradient( 0, 0, 0, 0, 0, 18 )
              .addColorStop( 0, 'rgba( 0, 0, 255, 200 )' )
              .addColorStop( 0.9, 'rgba( 0, 0, 255, 0 )' )
          } )
        ],
        spacing: 5
      } )
    );

Before doing anything, I'll bring this up at developer meeting is see if there are objections or other thoughts.

pixelzoom commented 2 years ago

Here's are the UI components whose constructors would need to be modified. I'm not planning to modify sim-specific subclasses, except where noted.

sun:

scenery-phet:

zepumph commented 2 years ago

Devs at today's meeting agree! Notes in the dev meeting doc.

pixelzoom commented 2 years ago

Notes in the dev meeting doc:

CM: Please review https://github.com/phetsims/sun/issues/769 and note objections/consensus.

  • This seems, really nice. Thanks!
pixelzoom commented 2 years ago

In the above commits, the constructor signature of Checkbox and its subclasses was changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signature of ComboBoxDisplay was changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signature of ThermometerNode was changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signature of TemperatureAndColorSensorNode was changed.

pixelzoom commented 2 years ago

In the above commit, the constructor signature of ZoomButtonGroup was changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signature of Slider was changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signatures of PageControl, BoogleanToggleNode, BooleanRectangularToggleButton, and BooleanRoundToggleButton were changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signatures of RectangularMomentaryButton and RoundMomentaryButton were changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signatures of RectangularStickyToggleButton and RoundStickyToggleButton were changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signatures of RectangularToggleButton and RoundToggleButton were changed.

pixelzoom commented 2 years ago

In the above commits, the constructor signature of ComboBox and its subclasses was changed.

pixelzoom commented 2 years ago

I've completed all of the refactors identified in https://github.com/phetsims/sun/issues/769#issuecomment-1159512550. WebStorm's "Change signature" feature made this really easy. What made it painful and time-consuming was the amount of time it took to run pre-commit hooks.

Slack#dev-public:

PSA: I’ve completed the revisions for constructor signatures that are listed in https://github.com/phetsims/sun/issues/769. In general, the Property associated with a UI component is the first argument, followed by more “verbose” arguments (content node, items, etc.)

Closing.