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

replace VerticalAquaRadioButtonGroup and VerticalCheckBoxGroup with layout managers #30

Closed
pixelzoom opened this issue Oct 29, 2013 · 10 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 29, 2013

Add layout managers, either in scenery, sun or scenery-phet, to make these groups go away.

Or clean up these groups, and generalize to one 'vertical group' that is independent of component type. There is duplicate code here.

@pixelzoom
Copy link
Contributor Author

I have to say... This got even worse after it was instrumented for PhET-iO.

For example, in VerticalCheckBoxGroup, the tandem for each check box is set like this:

65  tandem: options.tandem.createTandem( items[ i ].tandemName || 'checkBox' )

If I had used this in BLL, the tandemName for each item is the same as the solution name. So there would be no "CheckBox" in any of the tandem id for the check boxes. Whereas if I had created individual CheckBox instances, I would have given them tandem ids that have a "CheckBox" suffix (e.g. "showLinesCheckBox").

There's a similar problem in tandem ids for the AquaRadioButtons created by VerticalAquaRadioButtonGroup.

@samreid Do you agree that this is a little messed up?

@samreid
Copy link
Member

samreid commented Apr 27, 2017

We struggled with the same problem in ComboBox and settled on a similar strategy:

itemNodeTandem = options.tandem.createTandem( itemNodeOptions.tandemName || 'comboBoxItemNode' );

Forces and Motion: Basics uses VerticalCheckBoxGroup like so:

    this.verticalCheckBoxGroup = new VerticalCheckBoxGroup( [
      {
        content: new Text( sumOfForcesString, _.extend( { tandem: tandem.createTandem( 'showSumOfForcesTextNode' ) }, fontOptions ) ),
        property: model.showSumOfForcesProperty,
        tandemName: 'showSumOfForcesCheckBox'
      },
      {
        content: new Text( valuesString, _.extend( { tandem: tandem.createTandem( 'showValuesTextNode' ) }, fontOptions ) ),
        property: model.showValuesProperty,
        tandemName: 'showValuesCheckBox'
      },
      {
        content: showSpeedContent,
        property: model.showSpeedProperty,
        tandemName: 'showSpeedCheckBox'
      }
    ], {
      tandem: tandem.createTandem( 'verticalCheckBoxGroup' )
    } );

If I had used this in BLL, the tandemName for each item is the same as the solution name.

Perhaps we should document (and possibly have assertions?) that the item tandems should end in "CheckBox" for VCBG and "RadioButton" for VARBG?

@samreid samreid assigned pixelzoom and unassigned samreid Apr 27, 2017
@pixelzoom
Copy link
Contributor Author

In ComboBox, it's the item tandem ids that are automatically created. So we end up with ids like solutionCheckBox.drinkMix, which seems perfectly reasonable.

@samreid
Copy link
Member

samreid commented Apr 27, 2017

Why would it be reasonable to have solutionComboBox.drinkMix but not solutionRadioButtonPanel.drinkMix?

@pixelzoom
Copy link
Contributor Author

It should be solutionRadioButtonPanel.drinkMixRadioButton.

@samreid
Copy link
Member

samreid commented Apr 27, 2017

I'm confused, we should chat!

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 27, 2017

If you created your own CheckBox inside of a Panel, the id for the Panel would presumably be something like 'forcesControlPanel', and the CheckBox would be 'forcesControlPanel.showAppliedForceCheckBox', not 'forcesControlPanel.showAppliedForce'.

@samreid
Copy link
Member

samreid commented Apr 27, 2017

As pointed out in #30 (comment) the tandems end in showSpeedCheckBox not showSpeed. That is as-desired, right? So what is the problem?

@pixelzoom
Copy link
Contributor Author

OK, I see... The client is responsible for the "*CheckBox" name.

@pixelzoom
Copy link
Contributor Author

This doesn't appear to be a problem anymore, now that VBox is used. Closing.

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

No branches or pull requests

2 participants