phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Type problems for Screen homeScreenIcon and navigationBarIcon #902

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Noted while adding dispose to ScreenIcon in https://github.com/phetsims/joist/issues/901...

In Screen.ts, ScreenOptions includes:

  homeScreenIcon?: ScreenIcon | null;
  navigationBarIcon?: ScreenIcon | null;

But then the fields in Screen are:

  public readonly homeScreenIcon: Node | null;
  public navigationBarIcon: Node | null;

And then the default homeScreenIcon is not even a ScreenIcon`:

    // Create a default homeScreenIcon, using the Screen's background color
    if ( !options.homeScreenIcon ) {
      options.homeScreenIcon = new Rectangle( 0, 0, MINIMUM_HOME_SCREEN_ICON_SIZE.width, MINIMUM_HOME_SCREEN_ICON_SIZE.height, {
        fill: options.backgroundColorProperty.value,
        stroke: 'black'
      } );
    }

So as soon as I add dispose to ScreenIcon, it's no longer structurally the same as Node, and a bunch of new tsc errors occurs, spanning multiple repos.

It looks like @samreid converted Screen to TypeScript. I'll take a stab at fixing this, then will ask him to review.

pixelzoom commented 1 year ago

Fixed in the above commits. @samreid please review.

Summary:

samreid commented 1 year ago

I reviewed the commits, and everything looks good to me. Closing.