phetsims / calculus-grapher

"Calculus Grapher" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
4 stars 4 forks source link

String keys review #285

Closed marlitas closed 1 year ago

marlitas commented 1 year ago

from https://github.com/phetsims/calculus-grapher/issues/268

Two questions:

  1. What was the decision behind having two different keys predict and predictPreferences that point to the same value Predict?
  2. Are Lagrange and Leibniz keys capitalized because they're proper nouns? Does that trump JSON camel case formatting?
"Lagrange": {
    "value": "Lagrange:"
  },
  "Leibniz": {
    "value": "Leibniz:"
  },
  "variable": {
    "value": "Variable"
  },
pixelzoom commented 1 year ago
  1. What was the decision behind having two different keys predict and predictPreferences that point to the same value Predict?

It's not uncommon to duplicate strings when they have very different contexts, because translators or PhET-iO clients may want to use different strings for the different contexts. In the case of "Predict" - No one questioned labeling the radio button with "Predict". But there was discussion/concern about whether "Predict" was sufficient for labeling the control in Preferences. So I decided to provide flexiblility by providing different entries for the 2 contexts.

Back to @marlitas. If that satisfies your question and you agree that it's OK, please close. Otherwise let's discuss further.

pixelzoom commented 1 year ago

In the above commit, I documented this in PreditControl.ts:

    // Note that this control has its own StringProperty, separate from the StringProperty used to label the 'Predict'
    // radio button. It's not uncommon to duplicate strings when they have very different contexts, because translators
    // or PhET-iO clients may want to use different strings for the different contexts. In the case of "Predict" -
    // No one questioned labeling the radio button with "Predict". But there was discussion/concern about whether
    // "Predict" was sufficient for labeling the control in Preferences.  So there are separate StringProperties for
    // each context. See https://github.com/phetsims/calculus-grapher/issues/285
    const labelText = new Text( CalculusGrapherStrings.predictPreferenceStringProperty, {
pixelzoom commented 1 year ago

Whoops, I only addressed half of this issue.

Re:

  1. Are Lagrange and Leibniz keys capitalized because they're proper nouns? Does that trump JSON camel case formatting?

I'm ambilent about this. The format of JSON is really dependent on the format of what's reading the JSON. But in the above commit, I've switched to camelCase. Where I would draw the line is single-characters, like "A", "B", "C" -- I definitely would not use "a", "b", "c" for their keys. I'll also note that Studio's search feature is case insensitive, so it will find "lagrangeStringProperty" or "LagrangeStringProperty".

@marlitas ready for review, close if OK.

marlitas commented 1 year ago

Back to @marlitas. If that satisfies your question and you agree that it's OK, please close. Otherwise let's discuss further.

works for me, and thanks for adding the documentation clarifying.

Looks good! closing.