Closed pixelzoom closed 2 years ago
Another case in LevelSectionButton.ts:
scoreDisplayConstructor?: any; //TODO https://github.com/phetsims/vegas/issues/102
scoreDisplayOptions?: any; //TODO https://github.com/phetsims/vegas/issues/102
As @jbphet noted in https://github.com/phetsims/vegas/issues/105#issuecomment-1103422693, it would be nice to remove this kind of thing:
// Valid values for scoreDisplayConstructor. These are the types that are relevant for this status bar.
// All constructors must have the same signature!
const VALID_SCORE_DISPLAY_CONSTRUCTORS = [
ScoreDisplayLabeledNumber, ScoreDisplayNumberAndStar
];
... and use an interface instead.
Then there's still the problem of how to say "this option must have a value that is a constructor with this signature".
This is the TypeScript syntax for indicating the presence of a constructor:
scoreDisplayConstructor?: { new( scoreProperty: IProperty<number>, providedOptions?: any ): Node };
I'm not sure what to do about the options type. And perhaps a factory function (closure) instead of passing around constructors would be clearer.
I have committed changes to address this issue. The basic idea is to have an option that is a creator function for the score display instead of providing a constructor and separate options. I had @samreid take a look at the approach before making the commits, and he said he found it reasonable.
This works, and is much more type safe, so it solves the problem where any
had to be used in several places. Having said that, I don't absolutely love this solution because it means that the client is now fully responsible for configuring the score display, whereas the previous implementation used some of the options from the status bars and merged them into the score display options, making the API perhaps a little easier to use.
An alternative that I considered was to create a factory object that could take a score display style as an enum value and options as a superset of all possible options for the various styles of score display. This struck me as cool, but potentially more time consuming to implement, so I went the route that I did as a bit of a compromise to get it done in a reasonable amount of time.
Assigning to @samreid to review.
@samreid - I also requested that QA run a regression test these changes on master, see https://github.com/phetsims/qa/issues/817.
I reviewed the commits and this seems a good direction to go in. I have 3 main feedback points:
Node
. There are many places where the return type is identified as specific subtypes of score displays, such as:
createScoreDisplay?: ( scoreProperty: IProperty<number> ) => ScoreDisplayLabeledNumber | ScoreDisplayLabeledStars;
and
// valid types for the score display in level selection buttons
type ValidScoreDisplayTypes =
ScoreDisplayLabeledNumber |
ScoreDisplayLabeledStars |
ScoreDisplayStars |
ScoreDisplayNumberAndStar;
However, none of the usage sites need to know those are score displays--all they need to know is they are scenery Nodes. So I tried this patch and it type checked OK:
If we (in the future?) need more fine-grained type information about those score display subtypes, consider creating a base class, type or interface for it. But Node
seems best until we need something more.
// nested options for score display
options.scoreDisplayOptions = merge( {
font: options.font,
textFill: options.textFill
}, options.scoreDisplayOptions );
It seems reasonable to pass through the main font and textFill as options to the createScoreDisplay
functions, if we want to keep that ability. But if that is more complicated, it seems OK to drop that feature.
I've incorporated @samreid's recommendation to broaden the return type for createScoreDisplay
(thanks for the patch @samreid !), and it looks good. I did some regression testing in a couple of sims, and everything looks fine.
I decided not to add the options for font and text fill to createScoreDisplay
, since it does indeed seem more complicated, as @samreid noted above.
As for regressions testing using MultiSnapshotComparison, I started looking into it and bailed. It didn't seem worth the time to me given that QA is already testing in https://github.com/phetsims/qa/issues/817.
I think that should cover it. Closing.
A couple of uses of
any
that need to be addressed, identical in FiniteStatusBar.ts and InfniteStatusBar.ts:scoreDisplayConstructor
should be a constructor, one of the values inVALID_SCORE_DISPLAY_CONSTRUCTORS
.scoreDisplayOptions
should be the options type that matchesscoreDisplayConstructor
.I'm not sure how to address this, will investigate.