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

Designer review comments/todos for PhET-iO featured #219

Closed
22 tasks done
arouinfar opened this issue Dec 17, 2019 · 16 comments
Closed
22 tasks done

Designer review comments/todos for PhET-iO featured #219

arouinfar opened this issue Dec 17, 2019 · 16 comments

Comments

@arouinfar
Copy link
Contributor

arouinfar commented Dec 17, 2019

For #205 @kathy-phet and I looked over the Intro screen in detail (and the Vectors & Drag screens to a lesser extent). We have several comments, questions, todos for @zepumph prior to providing the overrides. There are also a few general questions related to common code, so I'll open an issue for those items in the phet-io repo.

For brevity, I've mentioned/listed tandems for the just introScreen, but these generally would apply to all screens.

  • Using TimeControlNode (Use TimeControlNode for Play/Pause/Step buttons #216) would clean up the tree a lot. Currently, these controls are pretty scattered. (UPDATE: MK did this over in Use TimeControlNode for Play/Pause/Step buttons #216)

  • Why are normalMotionRadioButton and slowMotionRadioButton not part of a radioButtonGroup? When moving to TimeControlNode, would these end up in a common radioButtonGroup?
    UPDATE from MK: yes see Use TimeControlNode for Play/Pause/Step buttons #216

  • projectileMotion.introScreen.model.altitudeProperty and projectileMotion.vectorsScreen.model.altitudeProperty should be deleted. There aren't any altitude controls on these screens.

  • There are several elements that have a slider control in Studio, but the slider doesn't snap appropriately and is generally hard to use. We think it would be preferable for instructional designers to be setting these values in the sim itself, and propose removing the slider control from Studio.

  • projectileMotion.introScreen.model.cannonAngleProperty
  • projectileMotion.introScreen.model.cannonHeightProperty
  • projectileMotion.introScreen.model.initialSpeedProperty
  • projectileMotion.introScreen.model.gravityProperty (this should also be read-only, not sure if that will remove the slider or not)
  • projectileMotion.introScreen.view.zoomProperty
  • These should be read-only.
  • projectileMotion.introScreen.model.gravityProperty
  • projectileMotion.introScreen.model.numberOfMovingProjectilesProperty
  • projectileMotion.introScreen.model.projectileDiameterProperty
  • projectileMotion.introScreen.model.projectileDragCoefficientProperty
  • projectileMotion.introScreen.model.projectileMassProperty
  • projectileMotion.introScreen.model.measuringTape.basePositionProperty: m
  • projectileMotion.introScreen.model.measuringTape.tipPositionProperty: m
  • projectileMotion.introScreen.model.tracer.dataPointProperty: lots of data here. Is it possible to add units to these?
  • projectileMotion.introScreen.model.tracer.positionProperty: m
  • @kathy-phet prefers degrees to ° and thinks it may be easier to read in the data stream. Is it possible/easy to use degrees in the data stream, but keep ° in the visuals of the sim? (Applies to model.cannonAngleProperty on all screens.)

  • The documentation for projectileMotion.introScreen.model.fireEnabledProperty could be cleaned up a bit. Should we mention that the max number of projectiles in the air is 3?

  • The projectileMotion.introScreen.model.speedProperty controls the speed of animation, but the naming could be a bit clearer/more parallel to the related isPlayingProperty. Let's change speedProperty to playingSpeedProperty

  • (covered by Rename Tracer to DataProbe #222) The probe that reads data from the points along the trajectory is simply called the tracer, but a more informative name would be something like dataProbe which is a name we've decided to use in Natural Selection. Would it take a lot of restructuring to call this a dataProbe?

  • (see Explain and improve model.trajectoryGroup in studio #223) The projectileMotion.introScreen.model.trajectoryGroup is a bit hard to understand. Could you walk us through it?

  • Shouldn't the projectileMotion.introScreen.view.initialSpeedPanel should contain the projectileMotion.introScreen.view.initialSpeedNumberControl?

  • When projectileMotion.introScreen.view.initialSpeedNumberControl.visibleProperty is false, it is still possible to return tools to the invisible toolbox. This seems like a bug.

  • Rename projectileMotion.introScreen.view.projectilePanel to projectileMotion.introScreen.view.projectileControlPanel

  • projectileMotion.introScreen.view.projectilePanel is missing top-level opacityProperty, pickableProperty, and visibleProperty.

  • (see Factor out airResistanceCheckbox and dragCoefficient readout to separate Node #225) Group airResistanceCheckbox, airResistanceText, and dragCoefficientReadout into airResistanceControl or simply airResistance. (@kathy-phet do you have a preference for the name?)

  • Rename projectileMotion.introScreen.view.vectorsPanel to projectileMotion.introScreen.view.vectorsControlPanel

  • (see How to instrument vectorVisibilityProperties  #224) It's not really clear why projectileMotion.introScreen.view.vectorVisibilityProperties is in the view. This seems analogous to gravityAndOrbits.modelScreen.model.showGravityForceProperty (both toggle the visibility of a vector in the play area), which is in the model.

  • (see also How to instrument vectorVisibilityProperties  #224) The "VisibilityProperties" ending to projectileMotion.introScreen.view.vectorVisibilityProperties seems pretty close to visibleProperty, which is a bit of a loaded meaning. Perhaps we should name it something different? @kathy-phet can't remember what we decided for this.

  • (fixed in is ZoomButton over-instrumented? scenery-phet#573) Parts of the magnifying glass icon on the zoom buttons are instrumented, but should be uninstrumented.

  • projectileMotion.introScreen.view.zoomInButton.glassNode
  • projectileMotion.introScreen.view.zoomInButton.signNode
  • Could we get some more explanation of how projectileMotion.global.model.projectileObjectTypes and projectileMotion.global.model.projectileObjectTypes.companionlessObjectType work?

  • UPDATE from @zepumph: over in Add a range for Score.targetXProperty #192 @arouinfar and I thought that we should feature targeXProperty.

@arouinfar
Copy link
Contributor Author

Assigning to @kathy-phet to compare her notes before we assign to @zepumph.

@zepumph
Copy link
Member

zepumph commented Jan 23, 2020

I will take a first pass, and this doesn't need to wait for a @kathy-phet review.

@zepumph
Copy link
Member

zepumph commented Feb 15, 2020

There are several elements that have a slider control in Studio, but the slider doesn't snap appropriately and is generally hard to use. We think it would be preferable for instructional designers to be setting these values in the sim itself, and propose removing the slider control from Studio.

Just for clarity, I did this on all 4 screens.

@zepumph
Copy link
Member

zepumph commented Feb 19, 2020

projectileMotion.introScreen.model.tracer.dataPointProperty: lots of data here. Is it possible to add units to these?

I didn't feel like the dataPointProperty was the place to do this, so I added it to the documentation of DataPointIO. You can access that easily by navigating to dataPointProperty then in the control panel setting "IO Type Documentation" to "selected.

How does that look to you?

zepumph added a commit to phetsims/axon that referenced this issue Feb 19, 2020
zepumph added a commit that referenced this issue Feb 19, 2020
zepumph added a commit that referenced this issue Feb 19, 2020
zepumph added a commit that referenced this issue Feb 27, 2020
@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

The projectileMotion.introScreen.model.speedProperty controls the speed of animation, but the naming could be a bit clearer/more parallel to the related isPlayingProperty. Let's change speedProperty to playingSpeedProperty

Over in #216 we renamed speedProperty to isSlowMotionProperty. What do you think about that?

@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

Shouldn't the projectileMotion.introScreen.view.initialSpeedPanel should contain the projectileMotion.introScreen.view.initialSpeedNumberControl?

Agreed! I also removed the redundancy such that it looks like: projectileMotion.introScreen.view.initialSpeedPanel.numberControl

zepumph added a commit that referenced this issue Feb 27, 2020
@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

When projectileMotion.introScreen.view.initialSpeedNumberControl.visibleProperty is false, it is still possible to return tools to the invisible toolbox. This seems like a bug.

I believe you are talking about when projectileMotion.introScreen.view.toolboxPanel.visibleProperty is false. I agree! What a fun bug, fixed above.

@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

projectileMotion.introScreen.view.projectilePanel is missing top-level opacityProperty, pickableProperty, and visibleProperty.

Two thoughts here. One, as an FYI for communication, this means that the child components are being instrumented, but the parent panel is not, and is instead just a place holder for structure. Second is that I did the same for the vectorsControlPanel.

zepumph added a commit that referenced this issue Feb 27, 2020
@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

Could we get some more explanation of how projectileMotion.global.model.projectileObjectTypes and projectileMotion.global.model.projectileObjectTypes.companionlessObjectType work?

These objectTypes work much like Solutes in ph-scale, where they are basically just constants that have data about how an instance of that type would behave. So if you know that the model's selectedProjectileObjectTypeProperty is cannonball, I know what the defaults will be by looking at the initialState of projectileMotion.global.model.projectileObjectTypes.cannonball:

{
  "name": "‪Cannonball‬",
  "mass": 17.6,
  "diameter": 0.18,
  "dragCoefficient": 0.47,
  "benchmark": "cannonball",
  "rotates": false,
  "massRange": {
    "min": 1,
    "max": 31
  },
  "massRound": 0.01,
  "diameterRange": {
    "min": 0.1,
    "max": 1
  },
  "diameterRound": 0.01,
  "dragCoefficientRange": {
    "min": 0.04,
    "max": 1
  }
}

UPDATE: the companionless one is used in vectors/drag screens where you can't choose different types. That is the only (companionless) projectile type you can fire projectiles of.

Does that make sense?

@zepumph
Copy link
Member

zepumph commented Feb 27, 2020

I have gotten through each checkbox in the first comment. Many have separate issues that are still open, but some were just discussed/implemented here. All the checkboxes that don't have a check have comments above that I thought should be discussed further before checking them off. Over to you @arouinfar.

@arouinfar
Copy link
Contributor Author

@zepumph things are generally looking good.

I've reviewed the checked items which weren't moved to their own issues. I'll review the unchecked items and still-open spinoff issues next time @kathy-phet and I work on this together.

@zepumph
Copy link
Member

zepumph commented Feb 8, 2021

Note that phetsims/scenery-phet#540 is for any feedback you have about Keypad. The structure may change a bit because of some general keypad work I'm doing in phetsims/scenery-phet#283

@arouinfar
Copy link
Contributor Author

@zepumph I've opened #244 to log issues from our latest review.

There's only one unchecked item left in this issue, which should be a quick fix. Once addressed, we can probably close this issue since any remaining work is being tracked elsewhere.

projectileMotion.introScreen.model.tracer.dataPointProperty: lots of data here. Is it possible to add units to these?

I didn't feel like the dataPointProperty was the place to do this, so I added it to the documentation of DataPointIO. You can access that easily by navigating to dataPointProperty then in the control panel setting "IO Type Documentation" to "selected.

How does that look to you?

Looks good! One small change -- can you update the units of airDensity to kg/m^3? It's currently using "cu m" for cubic meters which is a bit non-standard in this context.

@zepumph
Copy link
Member

zepumph commented Feb 25, 2021

Looks good! One small change -- can you update the units of airDensity to kg/m^3? It's currently using "cu m" for cubic meters which is a bit non-standard in this context.

Done above

These items need units.

projectileMotion.introScreen.model.measuringTape.basePositionProperty: m
projectileMotion.introScreen.model.measuringTape.tipPositionProperty: m
projectileMotion.introScreen.model.tracer.positionProperty: m

That was already in the code:

And I added the one for projectileMotion.introScreen.model.tracer.positionProperty, but I actually don't think that units work for Vector2. I will look into that over in phetsims/dot#107, as it seems like Vector2s will want units in the future.

projectileMotion.introScreen.model.tracer.dataPointProperty: lots of data here. Is it possible to add units to these?

I added phetioDocumentation to see DataPointIO for units. Does that seem good to you?

Other than the feature add in phetsims/dot#107, anything else here?

@zepumph zepumph assigned arouinfar and unassigned zepumph Feb 25, 2021
@zepumph
Copy link
Member

zepumph commented Feb 25, 2021

Just kidding. Property already supports units, and I see them now implemented:

image

@arouinfar please close if there is nothing else here.

@zepumph
Copy link
Member

zepumph commented Jul 26, 2021

Looks like the last thing here was to have units on the measuringTape PhET-iO elements. I just re-confirmed in studio.

Note that

projectileMotion.introScreen.model.tracer.positionProperty was renamed to projectileMotion.introScreen.model.dataProbe.positionProperty

I think this is ready to close now. Please reopen if there is anything else.

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

3 participants