LevelSelectionButton should extend RectangularPushButton instead of containing one

[samreid]

While @zepumph and I were working on https://github.com/phetsims/phet-io/issues/926 we noticed that LevelSelectionButton contains a RectangularPushButton instead of extending it. This will lead to an incorrect phet-io API where "a button contains another button". This should be fixed.

@jbphet any ideas why it was done this way? Do you anticipate any problems if we switch to inheritance?

[jbphet]

It was done as composition instead of inheritance because it optionally includes an additional node that exists outside the bounds of the button (below it, to be specific) that depicts the best time.

[samreid]

I wonder if there is a better name for this than LevelSelectionButton because it isn't a button?

[jbphet]

LevelSelectionThingie :) ?

We could just go with LevelSelector. Short, sweet, clear.

[samreid]

It was done as composition instead of inheritance because it optionally includes an additional node that exists outside the bounds of the button (below it, to be specific) that depicts the best time.

Not sure if it is a best practice, but it would be possible to addChild on a RectangularPushButton. But I'm not sure if RectangularPushButton needs to control its child list (say if it ever sets this.children = [...]).

But I'm also fine to rename this LevelSelector or LevelSelectionNode. @jonathanolson can you please advise how to proceed for this issue, then reassign to me?

[jonathanolson]

Composition sounds preferred, presumably phet-io would want to handle this.

RectanglarPushButton should have permission to control its own children (as most view components would, in my opinion), so inheritance would be tricky to maintain.

[samreid]

@jonathanolson also said:

LevelSelectionNode maybe? "LevelSelector" sounds model-y a bit

[samreid]

I found there are already 3 other things called LevelSelectionNode, which show many buttons for selecting levels.

[samreid]

I renamed as described above. @jbphet anything else to do here?

[jbphet]

I'm not too keen on the "Item" portion - it seems gratuitous, though I understand that it is an attempt to avoid overlap with other nodes. How about LevelSelectorNode? Also, two of the existing instances of LevelSelectionNode are in sims that I'm responsible for. I'd be happy to change them to more specific names in order to avoid overlap if that works better.

Also, the header comment could use an update.

[samreid]

LevelSelectorNode and LevelSelectionNode suggest to me that they allow you to select any level, not just one particular level. What about one of the following: LevelOverviewNode LevelIcon LevelIconNode LevelSelectionIcon

[jbphet]

I think we have bigger fish to fry and this isn't worth more discussion. I can live with LevelSelectionItemNode. "A rose by any other name...". Closing.

[samreid]

There are still TODOs marked for this issue, discovered during https://github.com/phetsims/chipper/issues/946

[jbphet]

@samreid - I just took a look at this, and the TODO was added by you, and I believe it has already been addressed by the discussion above where we concluded that the inclusion of the button by composition, as opposed to inheritance, should be left as is and the type could be renamed if it was bothersome. So, I'm hitting this back into your court.

[samreid]

LevelSelectionButton changed to inheritance in https://github.com/phetsims/vegas/issues/59. I'll update the options accordingly.

[samreid]

I updated the options, closing.