Convert RectangularRadioButton and RectangularRadioButtonGroup to TypeScript

[zepumph]

I ran into this with https://github.com/phetsims/sun/issues/699, so I will convert, though perhaps not 100% because options really need to start with the base types of the button hierarchy.

[zepumph]

I know a lot of code is being converted to typescript, but I decided to create an issue to communicate the following thoughts.

1. This is not my repo, and I want to let @pixelzoom know of changes going on in the repo.
2. The creation of TButtonAppearanceStrategy and TContentAppearanceStrategy, which are just fledglings, and can likely be improved as the rest of the button hierarchy is converted.
3. The awareness that I left options types as any because it seems like the most efficient use of time until parent options are created and used.

[zepumph]

Feel free to close!

[pixelzoom]

2. The creation of TButtonAppearanceStrategy and TContentAppearanceStrategy, which are just fledglings, and can likely be improved as the rest of the button hierarchy is converted.

What was the motivation for adding appearance strategties? That's one of the architecuteral aspects of sun buttons that we have previously identified as "questionable" and should be revisited if we ever have the chance to do sun butttons 2.0. So I'm wondering why we're propagating that pattern to more UI components. I would suggest having @jbphet review this aspect, and comment on whether he thinks it's a good idea.

3. The awareness that I left options types as any because it seems like the most efficient use of time until parent options are created and used.

I'm happy to see type RadioButtonItem, because I can immediately use it in Geometric Optics. But since AquaRadioButtonGroup takes items: AquaRadioButtonGroupItem<T>[], I think it should be renamed to RectangularRadioButtonItem.

class RectangularRadioButtonGroup<T> extends LayoutBox. LayoutBox has been converted to TypeScript, and LayoutBoxOptions exists. So I don't see any reason to use options?: any in RectangularRadioButtonGroup.

[zepumph]

wondering why we're propagating that pattern to more UI components.

I don't think of this as propagating. The code has been here for a long, long time (in the button hierarchy). It didn't think it was much worry to write 10 lines of Typing code to make the vague classing more structured. The reality is no one is going to tear up buttons right now (there isn't the time), but I don't feel like we are substantially adding to the technical debt. You could even argue that you wouldn't want to recreate sun buttons (2.0) until all files are typesafe, to limit undesired errors in refactoring etc.

My main purpose was just to ping about their usage so you didn't have to re-invent the wheel when it came to typing those options in the supertypes (where they are defined).

@jbphet, as the author of these buttons, can you take a glance at TContentAppearanceStrategy, TButtonAppearanceStrategy, and note their existence. Please reopen if you see any issues you'd like to discuss.

So I don't see any reason to use options?: any in RectangularRadioButtonGroup.

I went for it, but after 30 minutes here I'm bailing. Right now, instead of using nested options, RectangularRadioButtonGroup uses _.pick to pull out top level options and pass them to the RectangularRadioButton. So either we need to have RectangularRadioButtonOptions to have the right schema in the Group, or we need to convert these options to a nested pattern (and probably both). Either of them is not in the scope of this issue for me right now, sorry.

I renamed RadioButtonItem-> RectangularRadioButtonItem though!

[pixelzoom]

RectangularRadioButtonGroup is definitely not ready for review. I ran into this yesterday while trying to replace merge for https://github.com/phetsims/chipper/issues/1224. Specifically, it has these problems:

• [x] Use of merge
• [x] Use of as any
• [x] Use of as Required<SelfOptions> & RectangularRadioButtonGroupOptions & { tandem: Tandem }
• [x] 2 uses of // @ts-ignore
• [x] defaultOptions has no type specified, so many default values are incorrectly added to defaultOptions (spacing, orientation, buttonContentXMargin, buttonContentYMargin, buttonContentXAlign, buttonContentYAlign, labelSpacing, labelAlign, a11yHighlightXDilation, a11yHighlightYDilation)
• [x] normalOptions has no type specified
• [x] It sets children but does not omit children from RectangularRadioButtonGroupOptions
• [x] Multiple references to contentArray in doc, which does not exist.
• [x] buttonContentXAlign should have type AlignBoxXAlign
• [x] buttonContentYAlign should have type AlignBoxYAlign
• [x] Tests for !item.hasOwnProperty( 'phetioType' ) when items have no such property

It could also benefit from these improvements:

• [x] defaultOptions could use a better name, or better yet...
• [x] defaultOptions should really be nested radioButtonOptions, so that _.clone is unnecessary (API change, so requires changes at call sites)
• [x] normalOptions could use a better name
• [x] It's unclear if inner class ButtonWithLayoutNode is still needed. Convert to a type? Replace with AlignBox? Use FlowBox functionality?...
• [x] The doc says "See sun.ButtonsScreenView for example usage", but there is no example in that screen.

@zepumph do you mind if I continue work on this? EDIT: @zepumph replies "yes" in Slack.

[pixelzoom]

• [ ] defaultOptions should really be nested radioButtonOptions, so that _.clone is unnecessary (API change, so requires changes at call sites)

Looking at current usages:

• 35 occurrences of extends RectangularRadioButtonGroup
• 37 occurrences of new RectangularRadioButtonGroup

So an API change will be costly. But imo worth it in the long run, and will certainly clean up RectangularRadioButtonGroup.

Another reason to do this is that nested options is the standard in other "groups". For example:

• AquaRadioButtonGroupOptions has radioButtonOptions
• VerticalCheckboxGroupOptions has checkboxOptions

[pixelzoom]

Repos that use RectangularRadioButtonGroup:

• [x] acid-base-solutions
• [x] area-model-common
• [x] bending-light
• [x] build-a-molecule
• [x] cck-black-box-study
• [x] cck-common
• [x] collision-lab
• [x] color-vision
• [x] density-buoyancy-common (buoyancy, density)
• [x] energy-forms-and-changes
• [x] energy-skate-park
• [x] equality-explorer
• [x] estimation
• [x] faradays-law
• [x] fluid-pressure-and-flow
• [x] fraction-comparison
• [x] fractions-common
• [x] function-builder
• [x] gas-properties
• [x] geometric-optics
• [x] gravity-and-orbits
• [x] greenhouse-effect
• [x] hookes-law
• [x] isotopes-and-atomic-mass
• [x] masses-and-springs
• [x] model-of-the-hydrogen-atom
• [x] my-solar-system
• [x] natural-selection
• [x] normal-modes
• [x] number-line-common
• [x] number-line-distance
• [x] number-line-integers
• [x] number-line-operations
• [x] number-play
• [x] pendulum-lab
• [x] plinko-probability
• [x] proportion-playground
• [x] ratio-and-proportion
• [x] rutherford-scattering
• [x] scenery-phet demo
• [x] states-of-matter
• [x] sun demo
• [x] unit-rates
• [x] vector-addition
• [x] wave-interference
• [x] xray-diffration

[pixelzoom]

In https://github.com/phetsims/sun/commit/b2e8f81c91ddac89e014aea48a638b35765a8352 above:

• RectangularRadioButtonGroupOptions were refactored to radioButtonOptions
• merge was replaced by optionize and combineOptions

In the other commits, call sites were adjusted to use radioButtonOptions, see https://github.com/phetsims/sun/issues/740#issuecomment-1176785501 for the list of usages.

[pixelzoom]

All of the work identified in https://github.com/phetsims/sun/issues/740#issuecomment-1176344517 has been completed. @zepumph since you did the initial conversion to TypeScript, would you mind reviewing? High-priority and blocking, since these were major changes, and RectangularRadioButtonGroup is widely used.

I should also note that I reviewed RectangularRadioButton. Other than the "appearance strategy" problems identified with TODO comments in the code, it looks good.

[zepumph]

I liked converting ButtonWithLayoutNode to a type

I liked the way you organized and simplified the SelfOptions and defaults.

I with that optionize worked better for nested options so you didn't need a ! to access items in radioButtonOptions. I don't have a recommendation or path forward, but it is noted by me!

It always scares me to see real Errors in code, like https://github.com/phetsims/sun/commit/20b9159d433a0f126a9826e6f892058c5031296c. I would only put those there if there was a good reason for them to not be assertions. Most likely this isn't the case, but perhaps just a couple more seconds to ask "should these be Errors" before moving on.

Changing an API like this is a tremendously daunting amount of work. It is so clearly and obviously an improvement. Thank you for all your hard work! It is greatly appreciated by the team.

[pixelzoom]

It always scares me to see real Errors in code, like https://github.com/phetsims/sun/commit/20b9159d433a0f126a9826e6f892058c5031296c. I would only put those there if there was a good reason for them to not be assertions. Most likely this isn't the case, but perhaps just a couple more seconds to ask "should these be Errors" before moving on.

Me too. That's why the Error was replaced with an equivalent assertion in that commit.

Thanks for the review. Closing.