Tandem and class names for the phMeterNodes

[samreid]

phetsims/tandem#267 revealed that some of the phMeterNodes are actually AccordionBox instances, but they are not named as such. Based on phetsims/tandem#267, I renamed the tandems, but did not want to rename the implementation classes until we confirm the desired behavior. @arouinfar can you please check the tandems and recommend:

• is pHMeterNodeAccordionBox an OK tandem?
• if not, what should it be?

[arouinfar]

@samreid pHMeterNodeAccordionBox seems reasonable. I thought about shortening it to pHMeterAccordionBox, but the first screen contains a pHMeterNode, so I think pHMeterNodeAccordionBox is more consistent.

I already see pHMeterNodeAccordionBox in master, and it looks good. Not sure if there is anything else for this issue, so back to @samreid.

[samreid]

Thanks, I renamed the file accordingly. Closing.

[pixelzoom]

pHMeterNodeAccordionBox is a little weird. But @arouinfar makes reasonable arguments, so I can certainly live with it.

I could also have lived with both of them being named pHMeterNode, because even the one that is an AccordionBox is logically a "pH Meter". And I think it's fine for there to be exceptions to the tandem suffix conventions when it makes sense.

[pixelzoom]

I will also note that anyone searching for pHMeterAccordionBox, which I did initially, is not going to find it.

[pixelzoom]

Since class name PHMeterNodeAccordionBox is sufficiently weird/redundant to cause someone (including my future self) to pause... I've added this documentaton to PHMeterNodeAccordionBox.js in the above commit:

 * NOTE: This class has the somewhat-redundant name PHMeterNodeAccordionBox because we have 2 pH meters in this
 * sim (see MacroPHMeterNode for the Macro screen), and we want them both to be discoverable in Studio by searching
 * for 'pHMeterNode'. So their tandem names are 'pHMeterNodeAccordionBox' and 'pHMeterNode'. The downside of this is
 * that someone who is familiar with tandem naming conventions will not find 'pHMeterAccordionBox'.
 * See https://github.com/phetsims/ph-scale/issues/238.

[pixelzoom]

Reopening.

As I said above:

Since class name PHMeterNodeAccordionBox is sufficiently weird/redundant to cause someone (including my future self) to pause...

This immediately came up in #242 (convert to TypeScript). After some head scratching, and cursing myself for allowing PHMeterNodeAccordionBox to be named as such...

Micro pH Meter does not allow changing pH; My Solutions pH meter does. To resolve TypeScript errors, I had to refactor PHMeterNodeAccordionBox into subclasses. And since we typically name an AccordionBox subclass using the pattern {{TITLE}}AccordionBox, it made sense to change the class names as follows:

PHAccordionBox (base class, whose title is 'pH')
  MicroPHAccordionBox (subclass for Micro screen)
  MySolutionPHAccordionBox (subclass for My Solutions screen)

It then made sense to change the tandem names to match. So the tandem names for the 3 screens are now as follows (Macro is unchanged):

phScale.macroScreen.view.pHMeterNode
phScale.microSolutionScreen.view.pHAccordionBox
phScale.mySolutionScreen.view.pHAccordionBox

In this case I feel strongly that clarity of code, and sticking to naming conventions, is more important than the Studio client being able to search for "pHMeterNode" and find all 3 meters. Hopefully that's OK with @arouinfar, back to her for review.

[arouinfar]

In this case I feel strongly that clarity of code, and sticking to naming conventions, is more important than the Studio client being able to search for "pHMeterNode" and find all 3 meters.

I completely agree. I think pHAccordionBox is conventional and human-readable, so I don't have any worries about clients interpreting the tandem name. It's also easily discoverable in the tree using the select feature in Studio.