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

Set backgroundColorProperty and keyboardHelpNode in IntroScreen #46

Closed
pixelzoom opened this issue Jun 28, 2022 · 1 comment
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 28, 2022

For code review #41 ...

In MeanShareAndBalanceScreen.ts:

    const options = optionize<MeanShareAndBalanceScreenOptions, SelfOptions, ScreenOptions>()( {
      backgroundColorProperty: MeanShareAndBalanceColors.screenBackgroundColorProperty,
      keyboardHelpNode: new SliderControlsAndBasicActionsKeyboardHelpContent()
    }, providedOptions );

In most PhET sims, each screen has a unique color and unique keyboard-help content. So those options feel inappropriate in this base class. I recommend:

  • Move backgroundColorProperty and keyboardHelpNode options to IntroScreenView.ts
  • Rename screenBackgroundColorProperty to introScreenBackgroundColorProperty

That will put you (or the next developer) in a better position to add screens to this sim.

This also brings up the question of whether MeanShareAndBalanceScreen serves any purpose. Because if you remove those options, it's not doing anything. I would probably delete MeanShareAndBalanceScreen.ts for now, and wait to see if a base class is needed when future screens are added.

@marlitas
Copy link
Contributor

MeanShareAndBalanceScreen has been deleted, and backgroundColorProperty and keyboardHelpNode options have been moved to the appropriate classes. Closing.

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

2 participants