phetsims / sun

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

Convert Popupable, Dialog, and Dialog subclasses to TypeScript. #763

Closed pixelzoom closed 2 years ago

pixelzoom commented 2 years ago

Before I do https://github.com/phetsims/vegas/issues/107 (GameInfoDialog), or convert any other Dialog subclasses (like OopsDialog) to TypeScript, it makes sense to convert Dialog. And that requires converting Popupable. But I'm not sure how to convert Popupable.

@jonathanolson could you convert Popupable? Then assign back to me, and I'll handle Dialog and its subclasses.

This issue blockes https://github.com/phetsims/vegas/issues/107.

jonathanolson commented 2 years ago

Popupable has gracefulBind on showPopup/hidePopup, followed by assertions that they are non-null. Unclear how "graceful" to be since that will error it out slightly later. @zepumph can you take a look after I convert?

jonathanolson commented 2 years ago

https://github.com/phetsims/sun/commit/58db7d00252f0710565f69cb54b723acf6f848a6 seems like where it was added.

jonathanolson commented 2 years ago

Converted in commits above.

pixelzoom commented 2 years ago

Popupable was a great name for a trait. But it now seems misleading, not appropriate for a Node subclass. Should it be renamed? (Popup? PopupNode?...)

jonathanolson commented 2 years ago

Popup/PopupNode definitely works if it's converted into a Node subclass.

pixelzoom commented 2 years ago

Ah... I commented without looking at the changes. Based on Slack conversation, I was under the impression that it was being converted to a Node subclass. But this is fine for my needs.

pixelzoom commented 2 years ago

In the above commits, I converted Dialog to TypeScript. I was forced to use any in two places, to avoid introducing a dependency on joist:

  //TODO https://github.com/phetsims/chipper/issues/1253 type Sim will introduce a dependency on joist
  sim?: any;

  private readonly sim: any; //TODO https://github.com/phetsims/chipper/issues/1253 type Sim will introduce a dependency on joist

These uses of any will be tracked in https://github.com/phetsims/chipper/issues/1253 (no-explicit-any typescript-eslint rule).

pixelzoom commented 2 years ago

Common-code subclasses of Dialog that have not been converted to TypeScript include:

pixelzoom commented 2 years ago

I converted everything except PreferencesDialog. It has many subcomponents that also need to be converted -- PreferencesTabs, PreferencesPanels, AudioPreferencesPanel, GeneralPreferencesPanel,…. So I'm going to put, and have @jessegreenberg handle PreferencesDialog when he converts the rest of joist/js/preferences/.

pixelzoom commented 2 years ago

Remaining work here:

zepumph commented 2 years ago

Popupable has gracefulBind on showPopup/hidePopup, followed by assertions that they are non-null. Unclear how "graceful" to be since that will error it out slightly later. @zepumph can you take a look after I convert?

What you have here seems totally good and fine with me.

PreferencesDialog has been converted to Typescript. Thanks all! Closing