code review for new UI components

[pixelzoom]

Significant modifications and new code were involved in #59 and #66. This issue is to review that work.

Components to be reviewed:

• [x] StatusBar - base type for status bar that appears at top of games
• [x] FiniteStatusBar - status bar for games with a finite number of challenges, replaces ScoreboardBar
• [x] InfiniteStatusBar - new status bar for games with an open-ended (infinite) number of challenges
• [x] RewardDialog - dialog that appear when the user reaches some score in an open-ended game
• [x] LevelSelectionButton - enhances and replaces LevelSelectionItemNode
• [x] ScoreDisplay* - 4 types that implement different ways of presenting the score, used in all of the above

The vegas demo was also heavily modified.

High priority, since there's an immediate need for this is games that being developed now (Equality Explorer, Area Model, Fractions, ...)

Assigning to @jonathanolson. If you don't have time, please consult with @ariel-phet. Link any related issues that you create to this issue.

[pixelzoom]

In case you were planning to dive right into this... Please hold off for a couple of hours, I'm making an API change.

[pixelzoom]

API changes completed. Status bars and LevelSelectionButton are now responsible for creating and managing their own score display. Feel free to proceed.

[jonathanolson]

• [x] (RE StatusBar): I'm curious about barHeight being a parameter, instead of an option. I would generally assume that most sims would want to use a default bar height?

[jonathanolson]

• [x] In StatusBar, new Rectangle( 0, 0, 1, 1 { can be simplified to new Rectangle( {, unless we are really using the 1x1 dimensions.

[pixelzoom]

Agreed, barHeight would be better as an option.

Fine with changing to new Rectangle( {. I tend to stick with new Rectangle(x,y,w,h,{ instead of using the pseudo-polymorphic constructors.

[pixelzoom]

Agreed, barHeight would be better as an option.

Btw... Note that barHeight is computed by the subtypes based on the UI components in the bar. If you want the client to be able to set (or use a default) barHeight, that's going to be a bunch of work.

About this on Slack, @jonathanolson said:

Ahh, never mind. I just don't want clients all specifying different heights.

[jonathanolson]

• [x] updateLayout (between FiniteStatusBaar and InfiniteStatusBar) has a lot of duplication. What if subtypes had to implement an abstract layout( leftEdge, rightEdge, centerY ), or something else that deduplicates options.dynamicAligment (which looks the same between the two)?

[pixelzoom]

What if subtypes had to implement an abstract layout( ...

I think that's worth doing.

[jonathanolson]

• [x] InfiniteStatusBar handles the scoreDisplay resizing, but FiniteStatusBar doesn't handle that. Is there a difference between how the two cases need to be handled, or should they do the same thing?

[pixelzoom]

InfiniteStatusBar handles the scoreDisplay resizing, but FiniteStatusBar doesn't handle that. I

Probably an oversight, I'll make it so.

[jonathanolson]

• [x] InfiniteStatusbar (in its disposal) does:

  if ( self.bar.hasListener( 'bounds', updateLayout ) ) {
  self.bar.off( 'bounds', updateLayout );
  }

I don't see references for this.bar (at least initially). Is there something I'm missing?

[pixelzoom]

I don't see references for this.bar ...

Typo, should ben this.barNode.

[jonathanolson]

• [x] Does FiniteStatusBar need a disposal of startOverButton? (The back button is being disposed on the infinite one).

[pixelzoom]

Does FiniteStatusBar need a disposal of startOverButton? (The back button is being disposed on the infinite one).

Since the buttons are owned by the status bars, and not visible as part of the public API, neither button probably needs to be disposed.

[jonathanolson]

• [x] The one case of lineWidth: 1 in LevelSelectionButton can be removed, as it is the default.

[jonathanolson]

• [x] Similarly the { fill: null } is not needed in LevelSelectionButton's createSizedImageNode (can just omit the options argument).

[jonathanolson]

ScoreDisplayLabeledStars:

• [x] Doesn't seem to have disposeScoreDisplayLabeledStars. Maybe our vegas demo should allow disposing everything to verify disposal behavior? I can also verify.
• [x] Doesn't seem to dispose the child scoreDisplay (which we'd want).

[jonathanolson]

From slack:

Jonathan Olson [10:03 AM] Understood. Looks like I'll be able to integrate bar usage in area-model now?

Chris Malley [10:03 AM] that might be a good thing to attempt as part of code review.

I'll plan to integrate into area-model (and make-a-ten?) as part of the review then. Main pass through the code complete.

[pixelzoom]

All issues that @jonathanolson identified (see above check box items) have been addressed. @jonathanolson do you want to take a peek? If all looks OK, feel free to close.

[jonathanolson]

Looks good. I'll be integrating things today.