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

Convert TimeControlNode to TypeScript #737

Closed
pixelzoom opened this issue Apr 21, 2022 · 8 comments
Closed

Convert TimeControlNode to TypeScript #737

pixelzoom opened this issue Apr 21, 2022 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 21, 2022

Creating an issue for this one because it's relatively complicated, and it looks like some sim(s) will need review.

@pixelzoom pixelzoom self-assigned this Apr 21, 2022
pixelzoom added a commit that referenced this issue Apr 21, 2022
pixelzoom added a commit to phetsims/models-of-the-hydrogen-atom that referenced this issue Apr 21, 2022
pixelzoom added a commit to phetsims/gravity-and-orbits that referenced this issue Apr 21, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@pixelzoom
Copy link
Contributor Author

Done in the above commit. Assigning to @samreid for review because gravity-and-orbit required changes.

Bugs fixed in TimeControlNode:

  • SpeedRadioButtonGroup was setting options.radius, when it should have been setting options.radioButtonOptions.radius. I also added an example to ComponentsScreenView (see const speedTimeControlNode) to show that radio buttons are (by default) sizes to match their test labels.
  • SpeedRadioButtonGroup was propagating option touchAreaDilation, which does not exist in the super heirarchy.

Bugs fixed in GravityAndOrbitsTimeControlNode:

  • It was using speedRadioButtonGroupOptions.touchAreaDilation, which does not exist. Looking at old dev versions with ?showPointerAreas, this option was indeed ineffective.

Other changes to GravityAndOrbitsTimeControlNode:

  • It now uses TimeControlNodeOptions to correctly compose GravityAndOrbitsTimeControlNodeOptions.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 21, 2022

A couple more changes... The inner classes were getting large, their names weren't quite right, and they could be generally useful outside of TimeControlNode. So I factored them out as follows:

  • SpeedRadioButtonGroup -> js/TimeSpeedRadioButtonGroup.ts. (since this sets an EnumerationProperty)
  • PlayPauseStepButtons -> js/buttons/PlayPauseStepButtonGroup.ts (since "Group" is PhET's convention for a collection of related buttons)

There are a few nested options in TimeControlNode that should probably be changed to match the class names, but I'm wondering if it's worth the trouble:

  • speedRadioButtonGroupOptions -> timeSpeedRadioButtonGroupOptions
  • playPauseStepButtonOptions (note that the plurality of this name didn't match the original class name, should have been playPauseStepButtonsOptions) -> playPauseStepButtonGroupOptions

@samreid thoughts?

samreid added a commit to phetsims/gravity-and-orbits that referenced this issue Apr 22, 2022
@jbphet
Copy link
Contributor

jbphet commented Apr 22, 2022

@pixelzoom - There is a line of what looks like some debugging code in TimeSpeedRadioButtonGroup, it looks like this:

console.log( `AFTER: options.radioButtonOptions.radius=${options.radius}` );

I was going to remove it, but then thought you might still be using it, so I thought I'd just bring it to your attention.

@samreid
Copy link
Member

samreid commented Apr 23, 2022

I reviewed TimeControlNode and tested the usage in Gravity and Orbits. Everything looks good. I made 2 minor commits. I was unfamiliar with the 2nd type parameter in this optionize call, can you add a code comment or comment here about the intent?

    const options = optionize<TimeControlNodeOptions,
      Omit<SelfOptions, 'playPauseStepButtonOptions' | 'speedRadioButtonGroupOptions'>, NodeOptions>()( {

Does it mean we don't provide defaults for those options?

@samreid samreid assigned pixelzoom and unassigned samreid Apr 23, 2022
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Apr 24, 2022

There is a line of what looks like some debugging code in TimeSpeedRadioButtonGroup,

@jbphet or @samreid yes, please remove the console.log in TimeSpeedRadioButtonGroup. Or I'll handle it when I return.

I was unfamiliar with the 2nd type parameter in this optionize call, can you add a code comment or comment here about the intent?

@samreid there's no need for a code comment here. This is a common pattern that PhET developers need to become familair with. It's how nestOptions are treated so that you don't need to provide a null or {} default value. It was discussed recently in the Slack typescript channel. If you don't recall it, or don't understand it, please create a GitHub issue to discuss. But it occurs in many places, so a comment here is definitely not needed or desirable.

EDIT: Issue created, see phetsims/phet-core#127

@samreid
Copy link
Member

samreid commented Apr 25, 2022

In the commit, I also corrected some backwards <==> forwards metadata problems discovered in https://github.com/phetsims/phet-io/issues/1815

@samreid
Copy link
Member

samreid commented Apr 25, 2022

@jbphet or @samreid yes, please remove the console.log in TimeSpeedRadioButtonGroup.

I removed that in prior commits.

@samreid
Copy link
Member

samreid commented May 5, 2022

Review complete, closing.

@samreid samreid closed this as completed May 5, 2022
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