phetsims / a11y-research

a repository to track PhETs research into accessibility, or "a11y" for short
MIT License
3 stars 0 forks source link

Update A11yStrings conventions and practices to make it easier to convert to i18n later #65

Closed zepumph closed 6 years ago

zepumph commented 6 years ago

We know that we will be offering a11y strings to translators at some point, we just don't know when. The more that we can do now to sync up our practices of a11y strings to the way we use normal strings, the easier that conversion will be. Right now a11y strings are not in the same format as their json counterpart:

a11y:


  var OhmsLawA11yStrings = {
    resistanceUnitsPatternString: '{{value}} Ohms',
    voltageUnitsPatternString: '{{value}} Volts',
    resistanceSliderLabelString: 'R, Resistance',
...
}

normal:

{
  "acid": {
    "value": "Acid"
  },
  "acid-base-solutions.title": {
    "value": "Acid-Base Solutions"
  },
  "base": {
    "value": "Base"
  },

I think that there are a few things that we can do to make it easier on us later.

  1. Make sure that all a11y strings for a file are declared as vars at the top of the page. This is already being done for the most part, but every now and then we find one inline. Since i18n strings will need to be required, a block at the top, with variable declaration, will bridge the transition quite well. Example of bad practice (we've all done it): accessibleLabel: BASEA11yStrings.wallLabelString,

  2. Convert from an object schema {{key}}: {{string}} to {{key}}: { "value": {{string}} }. This one would require a fair bit of work, going through each a11y string so far, but we will need to do that eventually, so I say lets bite the bullet now. This will require visiting all the call sites and adding ".value" to the end of the variable declarations. Kinda sloppy, and I would like @jessegreenberg's opinion before proceeding.

Then when it is time to do the transfer, all we will need to do is:

  1. change variable declarations to use require and the string! plugin (removing the ".value" from the end of the line.
  2. Move all of the a11y strings into the normal strings json files. This should just be copying verbatim code.

I'm happy to do this grunt work now, especially since it will save so much time later. I'll just wait for @jessegreenberg to comment/give the go ahead. Especially since it would involve touching sims that he is actively working on like BASE.

zepumph commented 6 years ago

@jessegreenberg messaged me on slack with this question:

Convert from an object schema {{key}}: {{string}} to {{key}}: { "value": {{string}} }

I am still not quite sure how this would work. So at declarations you would add .value to end like var greenBalloonLabelString = BASEA11yStrings.greenBalloonLabelString.value Then change the .js a11y string files to .json files?

I say:

You are close. Your declaration line aligns with what I was thinking. I do not recommend changing to .json files though. Instead the .js file would look like:

  var OhmsLawA11yStrings = {
    resistanceUnitsPatternString: {
      value: '{{value}} Ohms'
    },
    voltageUnitsPatternString: {
      value: '{{value}} Volts'
    },
    resistanceSliderLabelString: {
      value: 'R, Resistance'
    },
jessegreenberg commented 6 years ago

Thanks @zepumph, this sounds good. +1 for leaving as .js files. I agree that this will reduce our work load later on. Removing my assignment, but if you would like to split up the work please reassign to me.

zepumph commented 6 years ago

I'm going to convert scenery-phet strings now because I broke xss fuzzing yesterday when I added a few new object keys into that file.

zepumph commented 6 years ago

Another things we can do to help ourselves is name keys in the string files without the "String" suffix, but include those suffixes in the variable names in the js files. This is enforced with the string! plugin by eslint. Tagging @jessegreenberg so he is aware.

image

jessegreenberg commented 6 years ago

Roger roger.

zepumph commented 6 years ago

I think the most important part of this issue is that all the a11y devs recognize that we should be following these moving forward, I don't think that it is important to go back and fix usages just for this issue (since we will have to do it anyways).

Currently there are about half of the js string files using {{key}}: {object} and half using {{key}}: {string}. Marking for a11y meeting so we can get on the same page and briefly discuss if this is worth looking into further.

jessegreenberg commented 6 years ago

Thanks @zepumph, understood that {{key}}: {object} should be used. I would recommend that we bite the bullet and change all existing string files to use {{key}}: {object} so that when other devs begin to follow this pattern by inspecting existing A11yString files, everything looks the same.

jessegreenberg commented 6 years ago

@zepumph did a search and found that these are the repos that need to be updated.

~- [ ] john-travoltage [@mbarlow12]~ ~- [ ] joist [@zepumph]~ ~- [ ] ohms-law [@zepumph]~ ~- [ ] gravity-force-lab-basics [@mbarlow12]~ ~- [ ] balloons-and-static-electricity [@jessegreenberg]~

~We will work on this together, dev names have been added to the repos that we will each work on.~

EDIT: We decided to create separate issues for this.

jessegreenberg commented 6 years ago

We have created new issues in each repo for this. Closing this issue.

jessegreenberg commented 6 years ago

Reopening to make sure that all a11y strings are loaded at the top of the files (like the string plugin). @zepumph volunteered to do this.

zepumph commented 6 years ago

The repos mentioned above (with separate issues) will cover those sims, but I will look at:

in this issue because they all already have the right structure for their string files.

mbarlow12 commented 6 years ago

Working through https://github.com/phetsims/gravity-force-lab-basics/issues/63, and I just wanted to confirm that we're NOT going the json and require route, correct?

zepumph commented 6 years ago

Correct, we are just setting ourselves up to make that easiest in the future. Thanks for checking.