phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

PhET-iO Studio Tree Review and Sign-Off #269

Closed marlitas closed 3 months ago

marlitas commented 3 months ago

We want to make sure the phet-io tree is fully polished and good to go before publishing. Over to @amanda-phet to review.

amanda-phet commented 3 months ago

In doing my review, I see this weird property that shouldn't exist, and doesn't seem to do anything: meanShareAndBalance.fairShareScreen.view.controls.syncButton

amanda-phet commented 3 months ago

First draft of overrides. Many of these make sense to adjust on the code side, since they are on multiple screens, or just make sense on the code side. I don't exactly recall the process, but I'm going to review this with Amy and then set up a meeting with @marlitas to discuss.

[Log] /* eslint-disable */ (studio.js, line 321)
window.phet.preloads.phetio.phetioElementsOverrides =
  {
    "meanShareAndBalance.balancePointScreen.model.maxKicksProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.balancePointScreen.model.meanFulcrumFixedProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.activeKickIndexProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.meanPredictionFulcrumValueProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.totalKickDistanceProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.tickMarksVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.numberDisplay.valueText.stringProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.playAreaNumberLineNode.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanWithRemainderProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.numberOfPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.predictMeanVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.snacksDistributedProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.totalSnacksProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.numberDisplay.valueText.stringProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.numberOfPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.totalSnacksProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.checkboxGroup": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.numberDisplay.valueText.stringProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.numberOfCupsProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.predictMeanVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.successIndicatorsOperatingProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.tickMarksVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.numberDisplay.valueText.stringProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.visibleProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.levelOutScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.meanPredictionSlider": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    }
  };
marlitas commented 3 months ago

Why is this here? meanShareAndBalance.balancePointScreen.view.notepadNode.balanceBeamNode.meanPredictionFulcrumSlider.trackNode.visibleProperty

That is part of common code instrumentation that we do not have control over in Slider. It is part of the refactoring work that needs to be done for PhET-iO to remove some of the over-instrumentation that happened in that past.

amanda-phet commented 3 months ago

Sorry, I forgot you created this issue! Those were kind of notes to myself to ask about later haha.

amanda-phet commented 3 months ago

Open questions:

  1. Some linked properties (e.g. meanShareAndBalance.balancePointScreen.view.sceneView.soccerBallNodes.soccerBallsEnabledProperty) aren't showing up in the Featured list. Double-check that these will show up after changes are made to the code, and I can run studio without the admin query parameter.
  2. Can we instrument the cueing arrows? [edit: this is already instrumented as showMouseCueProperty]
  3. In doing my review, I see this weird property that shouldn't exist, and doesn't seem to do anything: meanShareAndBalance.fairShareScreen.view.controls.syncButton [edit: added this to the list of other studio changes]
  4. meanShareAndBalance.balancePointScreen.view.kickButton should have a visibleProperty [edit: this is controlled by maxKicks, will add to examples.md]

Updated overrides list:

[Log] /* eslint-disable */ (studio.js, line 321)
window.phet.preloads.phetio.phetioElementsOverrides =
  {
    "meanShareAndBalance.balancePointScreen.model.meanFulcrumFixedProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.activeKickIndexProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.meanPredictionFulcrumValueProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.sceneModel.totalKickDistanceProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.tickMarksVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.numberDisplay.valueText.stringProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.balancePointScreen.view.playAreaNumberLineNode.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.maxPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.meanWithRemainderProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.numberOfPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.predictMeanVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.snacksDistributedProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.successIndicatorsOperatingProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.totalSnacksProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.distributeScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.maxPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.meanInfoPanelVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.numberOfPlatesProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.totalSnacksProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.model.totalVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.fairShareScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.maxCupsProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.meanProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.numberOfCupsProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.pipes.pipesOpenProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.predictMeanVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.successIndicatorsOperatingProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.model.tickMarksVisibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.checkboxGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.decrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.incrementButton.enabledProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.controls.pipeSwitch.toggleSwitch.visibleProperty": {
      "phetioFeatured": false
    },
    "meanShareAndBalance.levelOutScreen.view.controls.visibleProperty": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.meanPredictionSlider": {
      "phetioFeatured": true
    },
    "meanShareAndBalance.levelOutScreen.view.questionBar.visibleProperty": {
      "phetioFeatured": true
    }
  };
amanda-phet commented 3 months ago

@marlitas here is what I need from you!

marlitas commented 3 months ago
marlitas commented 3 months ago

@marlitas will bring the question about numberSpinnerVBox to dev meeting.

I think I was being dramatic about that. Since dev meeting was canceled today I went through the code to look at precedent, and there is tons of precedent for naming a class something other that VBox as the suffix if it extends a VBox. For example,GameIconNode, DemoScatterPlot, CurveManipulationWidthControl. Sorry about the unnecessary conversation, and it's a good lesson for me to understand more of the subtleties in our naming conventions.

The new name is now numberSpinnerControl.

marlitas commented 3 months ago

I believe I hit all of the override requests except the following (which @amanda-phet and I have already talked about).

"meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.decrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.incrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.balancePointScreen.model.sceneModel.activeKickIndexProperty": {
"phetioFeatured": false
}, ( set in soccer-common )
"meanShareAndBalance.balancePointScreen.view.controls.numberOfBallsSpinner.numberDisplay.valueText.stringProperty": {
"phetioFeatured": false
}, ( out of scope )
"meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.distributeScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.decrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.fairShareScreen.view.controls.numberOfPlatesSpinner.incrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
"meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.decrementButton.enabledProperty": {
"phetioFeatured": true
},( not possible in code )
"meanShareAndBalance.levelOutScreen.view.controls.numberOfCupsSpinner.incrementButton.enabledProperty": {
"phetioFeatured": true
}, ( not possible in code )
amanda-phet commented 3 months ago

Looks really good! I just have a few changes:

Question for @arouinfar : we can get that info with the array, but is there a reason not to also get it for each plate? This equivalent info is featured on Balance Point (inherited from soccer-common).

arouinfar commented 3 months ago

Question for @arouinfar : we can get that info with the array, but is there a reason not to also get it for each plate? This equivalent info is featured on Balance Point (inherited from soccer-common).

I think it's a good idea to feature these Properties.

I think we originally chose to not feature them because the array method would be more efficient/universal for API control. There's certainly no harm in featuring these Properties, and it could be helpful to instructional designers when setting up an initial state in Studio, especially with waterLevelProperty which is continuous.

amanda-phet commented 3 months ago

Thank you @arouinfar !

@marlitas can you address those three checkboxes above?

marlitas commented 3 months ago

Reviewed the above changes with @amanda-phet over zoom and we are ready to close. Yay!