search

phetsims / molarity

"Molarity" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/molarity
GNU General Public License v3.0
2 stars 6 forks source link

Unused capitalized solute amount strings #218

Closed jessegreenberg closed 4 years ago

jessegreenberg commented 4 years ago

In #217 I noticed that soluteAmountRegions.capitalized strings don't seem to be used in this sim right now. Both usages (screen summary and aria-valuetext) use lower case

@zepumph is this correct and if so can the capitalized form of strings be removed?

zepumph commented 4 years ago

I trust you if they aren't being used currently. I think that in general aria-valuetext is supposed to be lowercase, and alerts are supposed to be capitalized. @terracoda does it seem right to you the way regions are capitalized now? If so please close, otherwise assign back to @jessegreenberg.

terracoda commented 4 years ago

There were a lot issues around capitalization for this sim. The general strategy, is indeed, to follow what @zepumph said above.

If a region variable never appears at the beginning of a sentence in an alert (context response) or in the pdom (dynamic state description) then there would be no use of the capitalized strings for region names.

This seems very right to me, @jessegreenberg. Assign to me if you would like me to review in detail.

terracoda commented 4 years ago

@jessegreenberg, I verified in the A11y View. It all looks to good to me - no capitals needed on region names for Solute Amount or Solution Volume.

jessegreenberg commented 4 years ago

OK, thanks! Ill remove the capitalized strings and simplify the function since they are not used.

jessegreenberg commented 4 years ago

Inspecting the code it looks like the capitalized form used to be used when useQuantitativeDescriptionsProperty was true. But now none of these strings are used when quantitative descriptions are being shown. So it feels safe to remove.

jessegreenberg commented 4 years ago

This was moved into the next RC, closing since there is nothing specific for the qA team to verify.