phetsims / greenhouse-effect

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

Use camelCase for string keys #290

Open pixelzoom opened 1 year ago

pixelzoom commented 1 year ago

In greenhouse-effect-strings_en.json, there are quite a few string keys that violate PhET's camelCase convention, see below. Why the deviation from convention? This is unlikely to pass code review, see https://github.com/phetsims/phet-info/blob/master/checklists/code-review-checklist.md#internationalization. And they make the Studio tree look a bit odd, see screenshot below.

Recommended to change these to camelCase. E.g. `controlPanel.carbonMonoxide.

 "QuadWavelengthSelector.Microwave": {
    "value": "Microwave"
  },
  "QuadWavelengthSelector.Infrared": {
    "value": "Infrared"
  },
  "QuadWavelengthSelector.Visible": {
    "value": "Visible"
  },
  "QuadWavelengthSelector.Ultraviolet": {
    "value": "Ultraviolet"
  },
  "QuadWavelengthSelector.HigherEnergy": {
    "value": "Higher Energy"
  },

...

  "ControlPanel.CarbonMonoxide": {
    "value": "Carbon Monoxide"
  },
  "ControlPanel.Nitrogen": {
    "value": "Nitrogen"
  },
  "ControlPanel.Oxygen": {
    "value": "Oxygen"
  },
  "ControlPanel.CarbonDioxide": {
    "value": "Carbon Dioxide"
  },
  "ControlPanel.Methane": {
    "value": "Methane"
  },
  "ControlPanel.NitrogenDioxide": {
    "value": "Nitrogen Dioxide"
  },
  "ControlPanel.Ozone": {
    "value": "Ozone"
  },
  "ControlPanel.Water": {
    "value": "Water"
  },
  "ButtonNode.ReturnMolecule": {
    "value": "Reset Molecule"
  },
  "SpectrumWindow.buttonCaption": {
    "value": "Light Spectrum Diagram"
  },
  "SpectrumWindow.title": {
    "value": "Light Spectrum"
  },
  "SpectrumWindow.frequencyArrowLabel": {
    "value": "Increasing frequency and energy"
  },
  "SpectrumWindow.wavelengthArrowLabel": {
    "value": "Increasing wavelength"
  },
  "SpectrumWindow.radioBandLabel": {
    "value": "Radio"
  },
  "SpectrumWindow.microwaveBandLabel": {
    "value": "Microwave"
  },
  "SpectrumWindow.infraredBandLabel": {
    "value": "Infrared"
  },
  "SpectrumWindow.ultravioletBandLabel": {
    "value": "Ultra-\nviolet"
  },
  "SpectrumWindow.xrayBandLabel": {
    "value": "X-ray"
  },
  "SpectrumWindow.gammaRayBandLabel": {
    "value": "Gamma\nray"
  },
  "SpectrumWindow.visibleBandLabel": {
    "value": "Visible"
  },
  "SpectrumWindow.cyclesPerSecondUnits": {
    "value": "Hz"
  },
  "SpectrumWindow.metersUnits": {
    "value": "m"
  },
screenshot_2589
arouinfar commented 1 year ago

Thanks @pixelzoom, I agree. It's very strange title casing in the tree like this. @jbphet please use camel case.

pixelzoom commented 1 year ago

I'm a little concerned that changing some of these will impact existing translations -- for Greenhouse Effect, and Molecules and Light. So while this would be easy for me to do, I'm going to defer to @jbphet.

arouinfar commented 1 year ago

I'm a little concerned that changing some of these will impact existing translations

Oh, good point @pixelzoom. I don't think this is worth breaking translations. Might be best to just leave this as-is and be more careful of casing moving forward.

pixelzoom commented 1 year ago

@jbphet and I discussed this. He thinks it's worth reworking the strings that are specific to Greenhouse. The strings that are related to Molecules and Light should be left alone - maybe even moved back into the molecules-and-light repo "someday" per https://github.com/phetsims/greenhouse-effect/issues/271.

pixelzoom commented 1 year ago

Noting that this issue is related to this code review item for https://github.com/phetsims/greenhouse-effect/issues/331:

  • [ ] Make sure the string keys are all perfect. They are difficult to change after 1.0.0 is published. They should be literal, they should be explicit, and they should be consistent. ...
jbphet commented 1 year ago

I have two questions about this:

  1. When we say "use camelCase", are we objecting only to the examples shown above, which all begin with a capital letter (e.g. QuadWavelengthSelector.Microwave), or also to strings that use periods as dividers, such as temperature.units.valueUnitsPattern? The latter currently looks like this in the Studio tree: image 2 . Is it a bigger problem that strings show up in Studio that are not actually present in the sim? All of the strings shown above that start with a capital letter are from the Micro screen, i.e. the Molecules and Light sim, and don't appear in Greenhouse Effect yet. I am wondering if this is a general problem, so I looked at States of Matter Basics in Studio, and there are lots of strings in the tree there that aren't present in the sim, such as all of those that go with the "Interaction" screen. Should we log a general issue for this?
arouinfar commented 1 year ago

@jbphet to answer your questions...

  1. When we say "use camelCase", are we objecting only to the examples shown above, which with a capital letter (e.g. QuadWavelengthSelector.Microwave), or also to strings that use periods and dividers, such as temperature.units.valueUnitsPattern?

I have no objection to things like temperature.units.valueUnitsPattern since the pieces between the dividers use camelCase, like valueUnitsPattern. The problem is when the segments start with a capital letter.

2 . Is it a bigger problem that strings show up in Studio that are not actually present in the sim?

These strings will get stripped out during a build, so it's not an issue.

jbphet commented 1 year ago

Just above @arouinfar said, "I have no objection to things like temperature.units.valueUnitsPattern...[t]he problem is when the segments start with a capital letter". All of the strings that start with a capital letter are from the Molecules and Light sim and do not appear in the Greenhouse Effect simulation, and won't until we implement the "Micro" screen. There are other open issues that relate to the strings from Molecules and Light, specifically https://github.com/phetsims/molecules-and-light/issues/382 and https://github.com/phetsims/greenhouse-effect/issues/271. These other issues are currently deferred, since we aren't very clear at this point in time on the future of the "Micro" screen. I'm going to defer this issue also, and will plan to deal with all of these as part of a larger effort when we either implement the "Micro" screen or decide against it.