Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Factor out class NumberModelTypeControl. #58

Open
pixelzoom opened this issue Jan 6, 2025 · 1 comment
Open

Factor out class NumberModelTypeControl. #58

pixelzoom opened this issue Jan 6, 2025 · 1 comment
Assignees

Comments

@pixelzoom
Copy link
Collaborator

pixelzoom commented Jan 6, 2025

In NumberPairsPreferencesNode.ts, the code below should be factored out into class NumberModelTypeControl extends PreferencesControl. That will make it easier to address the PhET-iO instrumentation of this component, which is currently lacking/incorrect -- the control itself currently has no tandem and no visibleProperty, and numberModelRadioButtonGroup should be a child of that control.

    const numberModelRadioButtonGroup = new RectangularRadioButtonGroup( NumberPairsConstants.NUMBER_MODEL_TYPE_PROPERTY,
      [
        {
          createNode: () => new Text( 'Number Bond', PreferencesDialogConstants.CONTROL_LABEL_OPTIONS ),
          value: 'numberBondModel',
          tandemName: 'numberBondModelRadioButton'
        },
        {
          createNode: () => new Text( 'Bar Model', PreferencesDialogConstants.CONTROL_LABEL_OPTIONS ),
          value: 'barModel',
          tandemName: 'barModelRadioButton'
        }
      ], {
        orientation: 'horizontal',
        tandem: options.tandem.createTandem( 'numberModelRadioButtonGroup' ),
        isDisposable: false,

        // Hide or show the entire row, not just the radio button
        phetioVisiblePropertyInstrumented: false
      } );
    const numberModelControl = new PreferencesControl( {
      labelNode: new Text( 'Number Model Type', PreferencesDialogConstants.CONTROL_LABEL_OPTIONS ),
      controlNode: numberModelRadioButtonGroup
    } );

This will also clean up NumberPairsPreferencesNode.ts, which is already becoming a little difficult to read due to lack of whitespace between unrelated chunks of code:

    const numberModelRadioButtonGroup = new RectangularRadioButtonGroup( NumberPairsConstants.NUMBER_MODEL_TYPE_PROPERTY,
...
      } );
    const numberModelControl = new PreferencesControl( {
      labelNode: new Text( 'Number Model Type', PreferencesDialogConstants.CONTROL_LABEL_OPTIONS ),
      controlNode: numberModelRadioButtonGroup
    } );
    const secondLanguageControl = new SecondLanguageControl(
...
      } );
@pixelzoom
Copy link
Collaborator Author

pixelzoom commented Jan 6, 2025

Other things related to the implementation of this feature:

  • NumberModelType should be created.
  • NumberPairsConstants.NUMBER_MODEL_TYPE_PROPERTY should be a StringEnumerationProperty<NumberModelType>, not Property<string>.
  • Replace literals 'Number Bond' and 'Bar Model' with LocalizedStringProperty instances.

Rename to correspond to the Property type and UI labeling ("Number Model Type"):

  • const numberModelRadioButtonGroup => numberModelTypeRadioButtonGroup
  • const numberModelControl => numberModelTypeControl
  • tandem 'numberModelRadioButtonGroup' => 'numberModelTypeRadioButtonGroup'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants