BeakerNode should be responsible for setting its descriptionContent

[pixelzoom]

The chunk of code below is currently in MolarityScreenView. Since it's related to the beaker, it would be better-placed in BeakerNode. You'll need to add @param useQuantitativeDescriptions to BeakerNode's constructor.

    // a11y - updates PDOM beaker description when solute, concentration, or quantitative description properties change
    const getBeakerDescription = () => {

      // chemical formula pattern is the same for all solutes except drink mix.
      let chemicalFormulaPattern = StringUtils.fillIn( chemicalFormulaPatternString, {
        solute: soluteDescriber.getCurrentSolute(),
        chemicalFormula: soluteDescriber.getCurrentChemicalFormula()
      } );
      if ( soluteDescriber.getCurrentChemicalFormula() === MSymbols.DRINK_MIX ) {
        chemicalFormulaPattern = StringUtils.fillIn( drinkMixChemicalFormulaPatternString, {
          chemicalFormula: MSymbols.CITRIC_ACID
        } );
      }
      return StringUtils.fillIn( beakerDescriptionString, {
        solute: soluteDescriber.getCurrentSolute(),
        concentration: concentrationDescriber.getCurrentConcentration(),
        maxConcentration: StringUtils.fillIn( concentrationAndUnitString, {
          concentration: soluteDescriber.getCurrentSaturatedConcentration()
        } ),
        chemicalFormulaPattern: chemicalFormulaPattern
      } );
    };

    Property.multilink( [ model.solution.soluteProperty, model.solution.concentrationProperty, useQuantitativeDescriptions ], () => {
      beakerNode.descriptionContent = getBeakerDescription();
    } );

[twant]

Thanks for this! I moved that code into BeakerNode. I also added a few more params for the describers, in addition to useQuantitativeDescriptions. @zepumph, would love to review with you on Monday, and then I'll assign it back over to you, @pixelzoom

[zepumph]

@twant I feel like quite a bit has changed in the repo since May 24th, and @pixelzoom is no longer responsible dev. I think there are two good ways to proceed:

1. We can discuss this issue together the next time we pair, and review that way.
2. You can comment here on what you would like me to review, and we can do it async in this issue.

@pixelzoom please let us know if you still want to look at this issue, but if you don't have time we can take it from here.

[pixelzoom]

It's all yours!

[twant]

@zepumph do you want to keep this one open to discuss BeakerNode's descriptionContent specifically, or are you okay with discussing with the rest of the changed files in #112?

[zepumph]

I think it is fine to review right here since it is already set up for us.

• [x] In general it is idiomatic for tandem to be the last arg save options if it is there.

• [x] please mark MolarityBeakerDescriptionNode.updateBeakerSummaryString with visibility.

• [x] I think that MolarityBeakerDescriptionNode would be a little more approachable with some jsdoc for each method. Maybe even if it says updates the X bullet like "has {low} concentration". Can you think of anything better to add?

• [x] In MolarityBeakerDescriptionNode I am ok with the current implementation, in which every model change results in a complete rewrite of the beaker PDOM. Have you thought about making it a bit less naive, and only updating the bullets necessary when their specific dependencies change? This way is nice because it is simple, but I thought I'd mention an alternative that maximizes performance. The alternative could also be considered clearer, since it is more obvious which list item has which model dependencies.

[twant]

@zepumph updated the jsdoc (I like the examples of the strings that are being updated -- they help me most easily identify the bullet points that are changing) and the visibility on that method.

In terms of updating the PDOM, I tried out a version that updates specific bullet points with the corresponding properties. In doing so, I realized that many of the bullets depend on at least two properties, so it winds up being quite a bit of bullet points being updated on a given property update anyway. I think I'd prefer to keep the naive implementation for now, but happy to discuss tomorrow!

[twant]

@zepumph and I discussed and decided to close -- naive implementation seems preferable in this instance since performance isn't horribly affected and it's much cleaner.