phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Convert AquaRadioButtonGroup to TypeScript #732

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

Needed for https://github.com/phetsims/geometric-optics/issues/229.

Convert AquaRadioButtonGroup and its sun subclasses to TypeScript. The primary motivation for this is to replace the JSDoc for this constructor parameter:

 * @param { {node:Node,value:T,tandemName?:string,labelContent?:string}[]} items
 *   Each item describes a radio button, and is an object with these properties:
 *   node: Node, // label for the button
 *   value: *, // value associated with the button
 *   [tandemName: Tandem], // optional tandem for PhET-iO
 *   [labelContent: string] // optional label for a11y

with a type, like this:

type AquaRadioButtonGroupItem<T> = {
  value: T, // value associated with the button
  node: Node, // label for the button
  tandemName?: string, // name of the tandem for PhET-iO
  labelContent?: string // label for a11y
};

class AquaRadioButtonGroup<T> extends LayoutBox {
...
  constructor( property: Property<T>, items: AquaRadioButtonGroupItem<T>[], options?: AquaRadioButtonGroupOptions ) {
pixelzoom commented 2 years ago

Note that [tandemName: Tandem], // optional tandem for PhET-iO in the .js code is incorrect. tandemName is a string, used to create a Tandem.

pixelzoom commented 2 years ago

First order of business was to get tsconfig-module.json set up so that it recognizes assert, lodash, and qunit. @samreid helped me with that in https://github.com/phetsims/sun/commit/46bf9ba61ceba60c1d720afb555e4ac18e6f88b4. I also improved a couple of the comments in that file.

pixelzoom commented 2 years ago

In https://github.com/phetsims/sun/commit/7ea260f142658b442cf7ec748eaa9926fd8b8a06, I converted AquaRadioButtonGroup, VerticalAquaRadioButtonGroup, and HorizontalAquaRadioButtonGroup to TS. Despite the fact that git status told me that these would be renames, history was blown away for VerticalAquaRadioButtonGroup and HorizontalAquaRadioButtonGroup. Since they are thin convenience classes (all they do is set orientation:) I'm not going to spend the time to recover history. The important thing is that AquaRadioButtonGroup history was preserved.

I used the new type AquaRadioButtonGroupItem in geometric-optics, see https://github.com/phetsims/geometric-optics/commit/e7d7cdc4a364c1db266dc244029cff1fe780bb79.

I tested with a couple of JS sims (acid-base-solutions) and TS sims (geometric-optics, quadrilateral) and everything seemed OK. I'll leave this open for a bit, to make sure that nothing unexpected shows up in CT.

samreid commented 2 years ago

In 7ea260f, I converted AquaRadioButtonGroup, VerticalAquaRadioButtonGroup, and HorizontalAquaRadioButtonGroup to TS. Despite the fact that git status told me that these would be renames, history was blown away for VerticalAquaRadioButtonGroup and HorizontalAquaRadioButtonGroup

This dropped 72 commits from VerticalAquaRadioButtonGroup. I recommend restoring the history and tracking it with a rename. I'm happy to work on that if you wish.

pixelzoom commented 2 years ago

Go for it.

samreid commented 2 years ago

It was kind of messy, but I think the history is restored. If I renamed and made file changes, then IDEA showed a "rename" in the commit dialog, but a "delete and recreate" in the history. For those files, I had to rename without making any file changes. I'll add a dev meeting agenda item about the renaming history "gotcha".

pixelzoom commented 2 years ago

I did a git pull in sun, and I do see restored history for VerticalAquaRadioButtonGroup and HorizontalAquaRadioButtonGroup. Thanks @samreid.

pixelzoom commented 2 years ago

One final note, about options for AquaRadioButtonGroup. They are currently of type AquaRadioButtonGroupOptions, which is:

type AquaRadioButtonGroupOptions = Omit< any, 'children' >;

They should eventually be something like:

type AquaRadioButtonGroupOptions = {

  // passed to each AquaRadioButton that is created 
  radioButtonOptions?: AquaRadioButtonOptions,

  // Dilation of pointer areas for each radio button.
  // These are not part of radioButtonOptions because AquaRadioButton has no pointerArea options.
  // X dilation is ignored for orientation === 'horizontal'.
  // Y dilation is ignored for orientation === 'vertical'.
  touchAreaXDilation?: number,
  touchAreaYDilation?: number,
  mouseAreaXDilation?: number,
  mouseAreaYDilation?: number
} & Omit< LayoutBoxOptions, 'children' >;

... because AquaRadioButtonGroup extends LayoutBox, and sets children.

But we can't do that now because LayoutBox and AquaRadioButton have not been ported to TS, and (consequently) LayoutBoxOptions and AquaRadioButtonOptions do not exist. I asked @samreid if there was a convention for indicating that this work still need to be done (a //TODO or something), and he said something like "no, and it seems easily searchable". So I'm assuming that there will be some future task that deals with any across the code base.

So closing this issue.