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

Create a reusable Level-Selection UI. #108

Closed
pixelzoom opened this issue May 19, 2022 · 21 comments
Closed

Create a reusable Level-Selection UI. #108

pixelzoom opened this issue May 19, 2022 · 21 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented May 19, 2022

From #104 (comment).

  • Create a reusable Level-Selection UI for games.
  • Read the ?gameLevels query parameter, if it exists for the sim.
  • Use new layout features of scenery, if they are ready.
  • Identify sims that should use it.

The UI might include only the level-selection buttons, or it might include other UI optional components (title, info button, sound button, timer button,...)

It would be nice to have this done by mid June, for use in Number Play.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 31, 2022

This seems like an excellent place to try scenery's new GridBox. Here's an untested sketch.

type SelfOptions = {};
type LevelSelectionBoxOptions = SelfOptions & Omit<GridBoxOptions, 'children'>;

class LevelSelectionBox extends GridBox {

  constructor( buttons: LevelSelectionButton[], providedOptions?: LevelSelectionBoxOptions ) {

     const options = optionize< LevelSelectionBoxOptions, SelfOptions, GridBoxOptions>()( {
       // ...
     }, providedOptions );

     options.children = buttons;

     super( options );
  }
}

Althought this isn't really doing much .... Maybe wrap this in another class that adds title and optional Info button?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented May 31, 2022

As I think about this more, I'm moving closer to the conclusion that this UI component may not by desirable. There are too many things about it that can differ between sims, and (likely) little standardization in how things have been designed.

Parameters:

  • the title string, and its options
  • the icons for the level-selection buttons, and options for those buttons
  • layout of the buttons
  • optional Info button, the associated Info dialog, descriptions of each level
  • optional sound button and an associated Property
  • optional timer button and an associated Property
  • option to add additional buttons near the sound and timer buttons?
  • spacing between all of the above
  • margins

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jun 27, 2022

I thought I'd start by creating a subclass of GridBox to handle layout of LevelSelectionButtons, and be responsible for setting visibility of those buttons based on the gameLevels query parameter.

I ran into all kinds of problems with GridBox, which I reported to @jonathanolson in Slack, and in phetsims/scenery#1430. So this issue is on-hold until those problems are resolved.

Here's where I was with LevelSelectionBox.ts when I stopped:

LevelSelectionBox.ts

// Copyright 2018-2022, University of Colorado Boulder

/**

  • LevelSelectionBox provides the layout a set of LevelSectionButtons.
  • If the simulation supports the gameLevels query parameter (see getGameLevelsSchema.ts) the caller
  • can optionally provide options.gameLevels to control which buttons are visible.
  • @author Chris Malley (PixelZoom, Inc.)
    */

import { GridBox, GridBoxOptions } from '../../scenery/js/imports.js';
import vegas from './vegas.js';
import optionize from '../../phet-core/js/optionize.js';
import StrictOmit from '../../phet-core/js/types/StrictOmit.js';
import LevelSelectionButton from './LevelSelectionButton.js';

type SelfOptions = {

// Game levels whose buttons should be visible. Levels are numbered starting from 1.
// This is typically set to the value of the gameLevels query parameter. See getGameLevelsSchema.ts.
gameLevels?: number[];
};

export type LevelSelectionBoxOptions = SelfOptions & StrictOmit<GridBoxOptions, 'children'>;

export default class LevelSelectionBox extends GridBox {

public constructor( buttons: LevelSelectionButton[], providedOptions?: LevelSelectionBoxOptions ) {

const options = optionize<LevelSelectionBoxOptions, StrictOmit<SelfOptions, 'gameLevels'>, GridBoxOptions>()( {

  // GridBoxOptions
  autoColumns: 2, // the default is to put all buttons in one row
  xSpacing: 20,
  ySpacing: 20,
  children: buttons
}, providedOptions );

// Hide buttons for levels that are not included in gameLevels.
// We must still create these Nodes so that the PhET-iO API is not changed.
if ( options.gameLevels ) {
  assert && assert( _.every( options.gameLevels, gameLevel => ( Number.isInteger( gameLevel ) && gameLevel > 0 ) ),
    'gameLevels must be positive integers' );
  buttons.forEach( ( button, index ) => {
    button.visible = options.gameLevels!.includes( index + 1 );
  } );
}

super( options );

}
}

vegas.register( 'LevelSelectionBox', LevelSelectionBox );

@pixelzoom
Copy link
Contributor Author

@jonathanolson and I discussed a plan for addressing the GridBox issue in phetsims/scenery#1430.

@jonathanolson also suggested that I might consider a FlowBox with a maximum width, that wraps to the next line. That might be a better bit for the layout of LevelSelectionButtons, since we're typically want use multiple rows only if the layout gets too wide. But when hiding some buttons (either via ?gameLevels or via PhET-iO) we could end up with layout that are not aesthetically pleasing, like 4 button on the first row, and 1 button on the second row.

@pixelzoom
Copy link
Contributor Author

On hold until phetsims/scenery#1429 is addressed.

@pixelzoom
Copy link
Contributor Author

phetsims/scenery#1429 and phetsims/scenery#1430 have been addressed, so removing on-hold label.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 8, 2022

Above I said:

I thought I'd start by creating a subclass of GridBox to handle layout of LevelSelectionButtons, and be responsible for setting visibility of those buttons based on the gameLevels query parameter.

After running into problems with scenery layout Nodes, and rolling this around in my brain for a couple of weeks... I think that's the wrong approach, Coming up with a layout that makes everyone happy is unlikely, time consuming, etc.

So I'm thinking that a better approach would be to create LayoutSelectionButtonGroup, similar to other PhET "groups" (RectangularRadioButtonGroup, etc.), with a customizable layout.

LayoutSelectionButtonGroup would have:

  • support for gameLevels querty parameter
  • LayoutSelectionButtonGroupItem that describes a button (fields TBD, but patterned after other "item" types)
  • constructor param items: LayoutSelectionButtonGroupItem[]
  • option createLayoutNode: () => LayoutNode, with default that creates an HBox
  • options defaultHBoxOptions for use with default createLayoutNode, ignored if createLayoutNode is supplied
  • other common layouts can be supported as static functions that can be used as the value for createLayoutNode, and we can add these as needed (similar to the layoutFunction option for NumberControl)

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jul 13, 2022

  • I converted all of my sims to use LevelSelectionButtonGroup: fourier-making-waves, balancing-chemical-equations, reactants-products-and-leftovers, graphing-lines, graphing-slope-intercept. Other sims will be converted by their responsible developers.

The other sims are:

@kathy-phet consider choosing one of the above developers to review LevelSelectionButtonGroup. And as part of that review, ask them to convert one or more of their sims.

@chrisklus
Copy link
Contributor

@kathy-phet asked me to review this earlier today, self assigning.

@chrisklus chrisklus assigned chrisklus and unassigned kathy-phet Jul 13, 2022
@pixelzoom
Copy link
Contributor Author

Thanks @chrisklus. I realize that this is a long issue (lots of comments). You should be able to proceed by reading only these 4 comments, the first of which is a summary:

@chrisklus
Copy link
Contributor

LevelSelectionButtonGroup.ts is looking great and the gameLevels query parameter is working nicely. I had a good experience using this in Number Play. A couple notes:

  1. Can levelSelectionButtonOptions be optional? I ended up putting most options in item.options and could see how levelSelectionButtonOptions may not be used in some cases.

  2. Default layout got me pretty far but not quite what was in place in Number Play before, see Use custom layout in NumberPlayGameLevelSelectionButtonGroup number-play#184, and I did not see an easy way to fix this with a custom layout. I tried creating a type that tacks on info about the level (like game type) for the button so that I could split the buttons into two types for the layout, but naturally createLayoutNode is strict about its param type LevelSelectionButton[]. Any ideas on how to achieve this behavior? Or maybe it should not be a feature of LevelSelectionButtonGroup to know things about the levels the buttons are for? Worst case I could distinguish with getBaseColor from ButtonNode, but that seems not ideal. I'll add this as a comment in the issue so we can continue discussion over there.

Back to @pixelzoom for any changes/thoughts from the items above and feel free to close if all set.

@pixelzoom
Copy link
Contributor Author

  1. Can levelSelectionButtonOptions be optional?

Yes, good catch. Done in the above commit.

  1. Default layout got me pretty far but not quite what was in place in Number Play before ...

@chrisklus and I discussed this via Zoom, notes in phetsims/number-play#184 (comment).

So I think this is ready to close.

@pixelzoom
Copy link
Contributor Author

For the sims identified in #108 (comment), I've created sim-specific issues to convert to LevelSelectionButtonGroup. See issues linked above this comment.

@pixelzoom
Copy link
Contributor Author

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

5 participants