Closed pixelzoom closed 2 years ago
I immediately hit a problem with generalizing this.
Most game Info dialogs look like this one in Equality Explorer (InfoDialog.js). Each level description is a line of RichText, and they are arrangement in a single VBox.
And there is one string for each level description. For example, in equality-explorer-strings_en.json:
"level1Description": {
"value": "<b>Level 1</b> One-step equations"
},
"level2Description": {
"value": "<b>Level 2</b> One-step equations with negative coefficients"
},
"level3Description": {
"value": "<b>Level 3</b> Two-step equations"
},
"level4Description": {
"value": "<b>Level 4</b> Multi-step equations with fractions"
},
"level5Description": {
"value": "<b>Level 5</b> Multi-step equations with variables on both sides"
},
Number Play uses an entirely different layout and approach to translated strings. There are separate nodes for the level names vs their descriptions. And the names and descriptions are in separate VBoxes (see number-play/InfoDialog.ts).
And there are separate translated strings for level names vs descriptions:
"gameNameLevelNumberPattern": {
"value": "{{gameName}}: Level {{levelNumber}}"
},
"countFromPattern": {
"value": "Count from {{minimumChallengeNumber}}-{{maximumChallengeNumber}}"
},
"subitizeFromPattern": {
"value": "Subitize from {{minimumChallengeNumber}}-{{maximumChallengeNumber}}"
},
So I don't know how to proceed. Can we change the layout in Number Play to be like other sims? Can we change the approach to translated strings in Number Play?
Assigning to @chrisklus and @amanda-phet for feedback. I'm blocked until we determine a general approach that works for all sims.
Pinging @chrisklus and @amanda-phet. I'm still blocked on this feature.
Apologies for missing this. @amanda-phet would like me to make a recommendation.
So I don't know how to proceed. Can we change the layout in Number Play to be like other sims? Can we change the approach to translated strings in Number Play?
Seems like in order for this to work in common code, we need to do both. The main down side i see to this design-wise is that the non-bolded text the right will not be left aligned as it is now. The gap in between the two sets doesn't feel as important. But, I think those are acceptable tradeoffs in order to factor this code out.
@pixelzoom if I change the string structure to match what you outlined in EE, and then just delete the existing dialog, is that enough to unblock you on this issue?
@pixelzoom I completed those changes in the issue linked above. Let me know if that's what you were looking for.
Here are the new strings:
"countingLevel1Description": {
"value": "<b>Counting: Level 1</b> Count from 1-10"
},
"countingLevel2Description": {
"value": "<b>Counting: Level 2</b> Count from 11-20"
},
"subitizingLevel1Description": {
"value": "<b>Subitizing: Level 1</b> Subitize from 1-5"
},
"subitizingLevel2Description": {
"value": "<b>Subitizing: Level 2</b> Subitize from 6-10"
},
Noting that the 4 level descriptions can be accessed directly on the strings object, or you can get one for each level generally via the model: level.gameType.levelDescriptions[ level.levelNumber ]
Currently blocked by https://github.com/phetsims/sun/issues/763. Popupable and Dialog need to be converted to TypeScript.
In the above commits, I created GameInfoDialog. Of special note is the gameLevels
option, which callers can set to the value of their gameLevels
query parameter. (I did consider making the gameLevels
option a Property. And in fact, I went way down that path. But as something to control via PhET-iO, it seems unnecessary.)
I used GameInfoDialog in fourier-making-waves and equality-explorer, and it worked nicely.
For review, I think it would be productive to have @chrisklus do the review, and use GameInfoDialog in number-play (where I suggest renaming InfoDialog to something like NumberPlayGameInfoDialog).
Sims that use GameInfoDialog are blocked for publication until this review is completed.
In the above commit, I added a demo of GameInfoDialog to the vegas demo Components screen.
Thanks for these changes - GameInfoDialog looks great and is working well in Number Play. I tested various combos of the gameLevels
query parameter.
descriptionTextOptions
are providing the same font size, could we make that the default?vBoxOptions
are the same, and all are duplicating the two defaults in GameInfoDialog because maxWidth needs to be included with them. Since the maxWidth used is the same value for all usages, could we add it as a static default to GameInfoDialog (so it can still be used for the titleNode max width) and then use it in the default options?Patch of my proposed changes (not fully tested):
Removing the blocks-publication label. Back to @pixelzoom for any further changes if desired.
Another possibility for that second bullet could be to remove title
from GameInfoDialog options, add titleString
instead, and have GameInfoDialog be responsible for creating the titleNode, since those usages largely look the same. May be more trouble than it's worth though if we need the flexibility of exposing the Text options for the titleNode. However, this could also be an opportunity unify the phetio API for the title.
Thanks for the review, I'll have a look at the proposed patch.
Another possibility for that second bullet could be to remove
title
from GameInfoDialog options, addtitleString
instead, and have GameInfoDialog be responsible for creating the titleNode ...
We'd need to also add titleOptions
, for styling the Text created by GameInfoDialog. And someone will eventually want RichText instead of Text. And a useRichText
option to choose between Text and RichText, like we've been saddled with in NumberDisplay. And then that complicates titleOptions
-- what type do we give it? And then someone wants to use an icon in their title. And then... You see where I'm going here? title: Node
is general and future-proof. Dependency injection is a better fit for the title, at the expense of a tiny bit of duplicated work for (most?) callers. (And I wish we'd done this in most UI components that take a title: string
.)
- All usages of
descriptionTextOptions
are providing the same font size, could we make that the default?
Great suggestion. Done in the above commits.
- All usages of vBoxOptions are the same, and all are duplicating the two defaults in GameInfoDialog because maxWidth needs to be included with them. Since the maxWidth used is the same value for all usages, could we add it as a static default to GameInfoDialog (so it can still be used for the titleNode max width) and then use it in the default options?
Adding GameInfoDialog.DEFAULT_MAX_CONTENT_WIDTH
(as in the patch) still requires the caller to set maxWidth
for both their title
and vBoxOptions
(content). I decided to try a different approach, where GameInfoDialog is responsible for constraining its width, and were the caller (in most cases) never needs to deal with maxWidth
.
Here's a summary of what I did in the above commits:
Added GameInfoDialogOptions.maxContentWidth
, which defaults to 75% of the default ScreenView width. That seems to be a reasonable default (see screenshot below of Number Play with dev&stringTest=long
). And callers should hopefully never have to fiddle with this aspect of GameInfoDialog. But if they do, the option is there.
maxContentWidth
is used internally by GameInfoDialog, to set maxWidth
of the content (VBox) and a Node wrapper for the title. (If title were used elsewhere in the scenegraph via scenery's DAG feature, it would be rude to set title.maxWidth directly.)
vBoxOptions
now excludes maxWidth
, and maxContentWidth
is applied when the VBox is instantiated:
vBoxOptions?: StrictOmit<VBoxOptions, 'children' | 'maxWidth'>;
While it's now unnecessary for callers to set title.maxWidth
, it's OK if they do. If it's less than maxContentWidth
, it will have the desired effect and the title width will be further constrained. If it's greater than maxContentWidth
, then maxContentWidth
of the title's wrapper Node will take precedence, ensuring that the title does not get wider than the dialog's content. (This would be a nice feature to have in Dialog, but I don't have time to go there, Dialog has so many other problems, etc.)
@chrisklus ready for your review. Let me know if you think this is an acceptable solution. And feel free to close.
Commits look great, thanks. One note - In the above commits I removed all the duplicated defaults for vBoxOptions
, as that was part of my reasoning for creating a default for maxWidth separate from vBoxOptions, so those defaults no longer needed to be duplicated. Closing.
From https://github.com/phetsims/vegas/issues/104.
?gameLevels
query parameter, if it exists for the sim.It would be nice to have this done by mid June, for use in Number Play.