Convert ComboBoxItem from `class` to `type`

[pixelzoom]

I'd like to replace class ComboBoxItem with type ComboBoxItem:

type ComboBoxItem<T> = {
  value: T;
  node: Node;
  soundPlayer?: ISoundPlayer;
  tandemName?: string;
  a11yLabel?: string
};

A type would be more consistent with the pattern used for RectangularRadioButtonItem and AquaRadioButtonGroupItem. And I think that usage sites would read better.

Before proceeding, I'll bring this up at developer meeting to see if there are preferences/objections.

[zepumph]

Devs at today's meeting agree! Notes in the dev meeting doc.

[pixelzoom]

Notes from dev meeting doc:

CM: Please review https://github.com/phetsims/sun/issues/768 and note objections/consensus.

• This seems like a good fit here.
• One consideration is if we will ever want to serialize this data for PhET-iO, if that would ever be the case then this is not a good fit, since a class is easier to implement a toStateObject
• JG: what about the assertions in there?
• MK: Likely those would be moved to where the items are used in the list box.
• MS: Whenever you use a ComboBoxItem, (if other places than ComboBox) those assertions would need to use it too.

[pixelzoom]

One consideration is if we will ever want to serialize this data for PhET-iO, if that would ever be the case then this is not a good fit, since a class is easier to implement a toStateObject

I understand needing to serialize an ComboBoxItem's value or node, and that is already supported. But I'm having a hard time coming up with a use-case where we'd want to serialize the items. Like items in radio-button groups and checkbox groups, checkbox items are describe how to create the contents of the listbox. And I don't think serialization of that is necessary, or even desirable.

That said... If ComboBoxItem does need to be serialized, then is the same true for AquaRadioButtonGroupItem, RectangularRadioButtonItem, VerticalCheckboxGroupItem, ComboBoxDisplayItem, etc? And should they be converted from type to class?

So I see 2 options: (1) convert ComboBoxItem to a type (2) convert all "item" types to classes

@zepumph @samreid what is your recommendation?

[pixelzoom]

JG: what about the assertions in there?

The only assertion that isn't replaced by TypeScript type-checking is this one:

    assert && assert( !node.hasPDOMContent, 'pdomContent is set by ComboBox, use options.a11yLabel' );

@jessegreenberg Do other item types (eg AquaRadioButtonGroupItem) need this same assertion? Why or why not?

MK: Likely those would be moved to where the items are used in the list box. MS: Whenever you use a ComboBoxItem, (if other places than ComboBox) those assertions would need to use it too.

We definintely don't want to rely on this assertion having to be copied to multiple places for the same type of item. So if the !node.hasPDOMContent assertion is important, and is relevant for other "item" types, then that makes me lean toward option (2) above.

Adding @jessegreenberg to the discussion, please weigh in with your recommendation -- option (1) or (2)?

[zepumph]

One consideration is if we will ever want to serialize this data for PhET-iO, if that would ever be the case then this is not a good fit, since a class is easier to implement a toStateObject

We do not want to do this at this time, nor do I think that we will ever want to. The dev meeting conversation was more general about class vs types and why we may not want to convert/use a type over a class. This shouldn't block converting ComboBoxItem in my opinion.

[zepumph]

We definintely don't want to rely on this assertion having to be copied to multiple places for the same type of item.

Does it have to be in multiple spots? Can we just check it once at the top of the constructor of ComboBox?

[pixelzoom]

Does it have to be in multiple spots? Can we just check it once at the top of the constructor of ComboBox?

I think that's probably the case - one check in ComboBox. And it would certainly be preferrable to confirm !node.hasPDOMContent at the location where a11yLabel is being set. But this comment seemed to indicate otherwise, and I haven't investigated:

MS: Whenever you use a ComboBoxItem, (if other places than ComboBox) those assertions would need to use it too.

[pixelzoom]

This assertion currently appears in ComboBoxItem:

assert && assert( !node.hasPDOMContent, 'pdomContent is set by ComboBox, use options.a11yLabel' );

While investigating relocating this responsibility to ComboBox, I discovered that ComboBox does not set pdomContent. In fact, nothing in the entire sun repository sets pdomContent.

@jessegreenberg what am I missing here? Is the ComboBoxItem assertion vestigial? Is something missing in ComboBox?

[pixelzoom]

Hmmm... This makes converting ComboBoxItem to a type a bit more complicated. In ratio-and-proportion:

class ChallengeComboBoxItem extends ComboBoxItem<number> 

... and ChallengeComboBoxItem does some work that a type cannot.

ChallengeComboBoxItem is a bad design. The responsibility for setting the background color of the sim should not be in the item. The item should select a value, and setting the background color based on the value should be handled elsewhere. @zepumph may I change this?

What I said about ChallengeComboBoxItem is true in general. Items should not have responsibilities like this.

[zepumph]

Yes that makes sense. Go ahead and make that change. Thanks!

[zepumph]

But this comment seemed to indicate otherwise, and I haven't investigated:

That comment was stated generally as a potential worry, but ComboBoxItem only has one usage.

While investigating relocating this responsibility to ComboBox, I discovered that ComboBox does not set pdomContent. In fact, nothing in the entire sun repository sets pdomContent.

There is no pdomContent, it is just a check to see if there is something in the PDOM for the Node (the implementation is just reliant on tagName I believe. Does that make sense?

[zepumph]

@jessegreenberg and I just discussed this a bit more, and we feel totally fine about using types instead of classes. We do like that assertion though, and wished that it was in other item-like spots, for example over in https://github.com/phetsims/sun/blob/d2e9d3c15f8697543213dff713dbc7670b931374/js/buttons/RectangularRadioButtonGroup.ts#L264-L265

We think we should add that check where other a11y items objects are turned into the item classes that the view components use.

[pixelzoom]

@zepumph @jessegreenberg thanks for the feedback.

I'm going to address ChallengeComboBoxItem first in https://github.com/phetsims/ratio-and-proportion/issues/484. That is a pre-requisite to converting ComboBoxItem to a type.

[pixelzoom]

https://github.com/phetsims/ratio-and-proportion/issues/484 is done, so I can proceed with this.

[pixelzoom]

Summary of the above commits:

• Moved ComboBoxItem assertions to the beginning of ComboBox constructor
• Added type ComboBoxItem in ComboBox.ts
• Deleted ComboBoxItem.ts and class ComboBoxItem
• Converted all usages to type ComboBoxItem
• Add constant ComboBox.ITEM_TANDEM_NAME_SUFFIX = 'Item'
• Replaces 60 duplicated "Item" literals with ${ComboBox.ITEM_TANDEM_NAME_SUFFIX}

To Slack#dev-plublic:

PSA: I pushed changes to many repos for https://github.com/phetsims/sun/issues/768 (Convert ComboBoxItem from class to type).

@jbphet randomly selected to review.

[jbphet]

Changes look good to me. I also thought through the whole serialization thing (I think I'm the one who raised the question in the meeting), and I agree that we're very unlikely to ever need to serialize individual combo box items.

I also did a bit of regression testing on Molarity and States of Matter, since they both use ComboBox and have phet-io support, and things seemed to work just fine in both.

I found an unrelated problem with the sound while doing this review and created a separate issue for it (see link above). I'll follow up on that separately.

Closing.