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

Refine the studio tree #30

Closed
samreid opened this issue Jan 5, 2024 · 19 comments
Closed

Refine the studio tree #30

samreid opened this issue Jan 5, 2024 · 19 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 5, 2024

@matthew-blackman and I would like to review the tree. @pixelzoom also recommends looking at the design section of https://github.com/phetsims/phet-io/issues/1969#issuecomment-1753353643

We would also like to check in with @arouinfar once this is ready. @pixelzoom recommends this may be better sooner rather than later.

@samreid samreid added the dev:help-wanted Extra attention is needed label Jan 8, 2024
@matthew-blackman matthew-blackman self-assigned this Jan 19, 2024
samreid added a commit that referenced this issue Jan 22, 2024
samreid added a commit that referenced this issue Jan 22, 2024
samreid added a commit that referenced this issue Jan 23, 2024
matthew-blackman added a commit that referenced this issue Jan 23, 2024
@matthew-blackman
Copy link
Contributor

Note about the Stopwatch: If we decide to go with a draggable design for the stopwatch in #94, we may want to implement a more traditional StopwatchNode which requires a Stopwatch instance. Otherwise, the Stopwatch in the VSMModel can be removed.

@samreid
Copy link
Member Author

samreid commented Jan 24, 2024

@matthew-blackman and I completed our review and refinement for VSM. We still need to do Sampling.

@samreid
Copy link
Member Author

samreid commented Feb 5, 2024

All ideas implemented or moved to side issues. @arouinfar said she would like to phetioFeatured in the context of #29. Nice work @matthew-blackman @arouinfar and myself. Closing.

@matthew-blackman
Copy link
Contributor

Reopening with feedback from 4/5 meeting with @arouinfar:

  • Interval tool model properties should be featured.
  • Instrument the simPreferences UI
  • Create an 'audioPreferences' tandem for all audio preferences.

@matthew-blackman
Copy link
Contributor

Ready for your review @arouinfar. It looks like the sim preferences and audio preferences UI are already instrumented under projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.preferencesDialog.preferencesPanels. Can you confirm?

@arouinfar
Copy link

Looks good on main, thanks @matthew-blackman.

It looks like the sim preferences and audio preferences UI are already instrumented under projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.preferencesDialog.preferencesPanels. Can you confirm?

Yes, looks like it's all there. Not sure why things appeared to be missing during our review together!
image

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 8, 2024

Reopening. The API was not regenerated, failing git-hooks, so I regenerated in https://github.com/phetsims/phet-io-sim-specific/commit/0a37eb7bfbb17d8cd5bb4d5229fafaf2665b2fc3. And this change looks odd to me -- why is there an additional audioPreferences tandem inserted here?

screenshot_3194

@samreid
Copy link
Member Author

samreid commented Apr 8, 2024

From slack:


Chris Malley
  1 hour ago
After a pull-all, git-hooks are failing with:
projectile-data-lab BREAKING PROBLEMS
PhET-iO Element missing: projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.audioPreferencesPanel.launchSoundStrategyControl
PhET-iO Element missing: projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.audioPreferencesPanel.playLandingSoundControl
projectile-data-lab DESIGN PROBLEMS
PhET-iO Element missing: projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.audioPreferencesPanel.launchSoundStrategyControl
PhET-iO Element missing: projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.audioPreferencesPanel.playLandingSoundControl
New PhET-iO Element not in reference: projectileDataLab.general.view.navigationBar.preferencesButton.preferencesDialogCapsule.archetype.preferencesPanels.audioPreferencesPanel.audioPreferences
projectileDataLab.measuresScreen.model.intervalTool.edge1Property.phetioFeatured changed from "false" to "true"
projectileDataLab.measuresScreen.model.intervalTool.edge2Property.phetioFeatured changed from "false" to "true"
10 replies


Chris Malley
  1 hour ago
grunt generate-phet-io-api shows this rather odd addition to audioPreferencesPanel. Is this what you intended to do? 
@Matt Blackman
 
@samreid
screenshot_3194.png
 
screenshot_3194.png




Sam Reid
  1 hour ago
Yes, we matched that with something else and checked with 
@arouinfar
, if I recall correctly. I’ll double check and commit


Chris Malley
  1 hour ago
I pushed an updated projectile-data-lab-phet-io-api.json, so that I can continue working.
:+1:
1



Chris Malley
  1 hour ago
audioPreferencesPanel.audioPreferences  smells like a workaround for not being about to get audioPreferencesPanel to add sim-specific audio controls.


Chris Malley
  1 hour ago
When common controls like the “Audio Features” toggle are someday instrumented, will they be under `audioPreferencesPanel.audioPreferences . I think not.  And it’s not even possible because you’re creating that tandem in the sim. (edited) 


Amy Rouinfar
  1 hour ago
The structure mirrors sim preferences which are collected under simulationPreferencesPanel.simPreferences.


Chris Malley
  43 minutes ago
simulationPreferencesPanel is sim specific.  audioPreferencesPanel is not. How are controls that are common to all Audio panels going to be instrumented someday?  And if this is the convention for the Audio preferences panel, why is audioPreferencesPanel.audioPreferences created in the sim instead of joist?


Sam Reid
  41 minutes ago
I’ll be in a meeting for a bit, let me and 
@Matt Blackman
 know what you recommend.


Chris Malley
  39 minutes ago
It seems like this additional level that’s being added to *PreferencesPanel is a workaround for not having the panel’s tandem name — a joist deficiency.  For panels in general, we would never do particlesPanel.particleControls.numberOfParticlesSpinner , we would do particlesPanel.numberOfParticlesSpinner .  Why are we adding an intermediate tandem for Preferences panels?


Chris Malley
  38 minutes ago
This could be solved by adding:
SimulationPreferencesPanel.TANDEM_NAME
AudioPreferencesPanel.TANDEM_NAME
etc.

@matthew-blackman
Copy link
Contributor

@samreid and I discussed this with @pixelzoom and we are all in agreement that the 'simPreferences' and 'audioPreferences' tandems do not appear to be needed. We removed these tandem levels in the commit, and would like to open a common code issues to remove the 'simPreferences' tandem for the children of SimulationPreferencesContentNode.

Nice work! This issue can be closed.

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

4 participants