Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use camelCase for string keys #290

Open
pixelzoom opened this issue Jun 8, 2023 · 8 comments
Open

Use camelCase for string keys #290

pixelzoom opened this issue Jun 8, 2023 · 8 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 8, 2023

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
Copy link
Contributor

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

@arouinfar arouinfar removed their assignment Jun 9, 2023
@pixelzoom
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

pixelzoom commented Jun 12, 2023

@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 #271.

@pixelzoom
Copy link
Contributor Author

Noting that this issue is related to this code review item for #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. ...

@pixelzoom pixelzoom mentioned this issue Jul 13, 2023
80 tasks
@jbphet
Copy link
Contributor

jbphet commented Jul 14, 2023

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?

@jbphet jbphet assigned pixelzoom and arouinfar and unassigned jbphet Jul 14, 2023
@arouinfar
Copy link
Contributor

@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.

@arouinfar arouinfar assigned jbphet and unassigned arouinfar Jul 15, 2023
@jbphet
Copy link
Contributor

jbphet commented Jul 17, 2023

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 phetsims/molecules-and-light#382 and #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants