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

Why do <span> tags have inline styling? #76

Closed
liammulh opened this issue Feb 16, 2023 · 19 comments
Closed

Why do <span> tags have inline styling? #76

liammulh opened this issue Feb 16, 2023 · 19 comments
Assignees
Labels
status:blocks-sim-publication type:question Further information is requested

Comments

@liammulh
Copy link
Member

From Slack:

Liam Mulhall
4 minutes ago
Is it okay if a sim has <span> tags in its strings? I saw this when I went to the translation form for BAN:
ban-strings

Jonathan Olson
3 minutes ago
Seems fine in general, however I’m not sure why the style is in the translated string. I’d expect that to be hardcoded in the sim
👍
1

Liam Mulhall
3 minutes ago
It's a prototype, so they might change.

John Blanco
🏡 3 minutes ago
Probably worth a question issue in GitHub for whoever set the strings up that way.
👍
1

Liam Mulhall
2 minutes ago
Okay, I'll open up an issue in the BAN repo.

It looks like @Luisav1 and @marlitas are the responsible devs, so assigning them.

@liammulh liammulh added the type:question Further information is requested label Feb 16, 2023
@marlitas
Copy link
Contributor

After a short chat with @chrisklus it looks like this was done to create even spacing among the labels in the Half-Life Timescale Dialog.

Screenshot 2023-02-17 at 9 22 06 AM

I looked into changing this, and I do believe we can hardcode it through the sim by setting the monospace font when the Text element is created. This would require some code restructuring (the css is overwriting Text font options in a couple of places, which I don't love).

Before I continue moving forward with this:

  1. @liammulh / @jbphet What are the repercussions of changing string values when there is an already published prototype? Specifically when this sim goes to full publication?
  2. I want to check-in with @Luisav1 to also confirm the motivation/need and what I'm seeing in the code.

@liammulh
Copy link
Member Author

What are the repercussions of changing string values when there is an already published prototype? Specifically when this sim goes to full publication?

There is a big red warning in the translation utility that strings in prototype sims might change, so it's preferable to change the strings before publication rather than after.

@liammulh
Copy link
Member Author

What are the repercussions of changing string values when there is an already published prototype? Specifically when this sim goes to full publication?

To answer the original question, the translators will have to re-translate the string if they've already translated it. But assuming they've read the warning, they are okay with that.

@jbphet
Copy link
Contributor

jbphet commented Feb 17, 2023

What @liammulh says about the translators re-translating is true, but if you use the same string keys, this will look pretty bad in the translated versions. I had a similar issue come up for Greenhouse Effect, and I did a manual fix of all of the translations before re-publishing it. It was essentially just a search and replace of the strings, so it wasn't hard, but it had to be timed to be done just before the sim was re-published. You could log an issue and plan to do the same thing.

Alternatively, you could change the English strings, then fix the translations, and then publish a maintenance release of the prototype. That would solve the problem and there would be no risk of missing this step with the next publication.

@marlitas
Copy link
Contributor

@Luisav1 and I decided that since her time on this sim is limited we will put this issue on hold until the sim is ready for publishing.

@zepumph
Copy link
Member

zepumph commented Jun 27, 2023

We are getting ready to sprint on this. Marking off hold.

@zepumph
Copy link
Member

zepumph commented Jul 6, 2023

@Luisav1 and I discussed this today, and would like to experiment with making the first letter its own Node. Then we can use an AlignBox to ensure the correct spacing.

  • Let's make sure that this solution still works with translations that include the mono space string key.

@zepumph
Copy link
Member

zepumph commented Jul 21, 2023

This is blocking me testing stringTest=dynamic because of the html formatting. I'm going to work on this in correlation with #90. @marlitas recommended grid box with 3 columns: the letter, the hyphen, and the i18n sentence.

@zepumph zepumph self-assigned this Jul 21, 2023
@zepumph
Copy link
Member

zepumph commented Jul 21, 2023

Subject: [PATCH] dynamic locale adjustments to the "Available Decays" panel, https://github.com/phetsims/build-a-nucleus/issues/90
---
Index: build-a-nucleus-strings_en.json
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/build-a-nucleus-strings_en.json b/build-a-nucleus-strings_en.json
--- a/build-a-nucleus-strings_en.json	(revision 5bf96458622725cddb04daebfb801452771b14ff)
+++ b/build-a-nucleus-strings_en.json	(date 1689962899555)
@@ -36,64 +36,64 @@
     "value": "Half-Life Timescale"
   },
   "timeForLightToCrossANucleus": {
-    "value": " - Time for light to cross a nucleus"
+    "value": "Time for light to cross a nucleus"
   },
   "timeForLightToCrossAnAtom": {
-    "value": " - Time for light to cross an atom"
+    "value": "Time for light to cross an atom"
   },
   "timeForLightToCrossOneThousandAtoms": {
-    "value": " - Time for light to cross 1000 atoms"
+    "value": "Time for light to cross 1000 atoms"
   },
   "timeForSoundToTravelOneMillimeter": {
-    "value": " - Time for sound to travel 1 mm"
+    "value": "Time for sound to travel 1 mm"
   },
   "aBlinkOfAnEye": {
-    "value": " - A blink of an eye"
+    "value": "A blink of an eye"
   },
   "oneMinute": {
-    "value": " - One minute"
+    "value": "One minute"
   },
   "oneYear": {
-    "value": " - One year"
+    "value": "One year"
   },
   "averageHumanLifespan": {
-    "value": " - Average human lifespan"
+    "value": "Average human lifespan"
   },
   "ageOfTheUniverse": {
-    "value": " - Age of the Universe"
+    "value": "Age of the Universe"
   },
   "lifetimeOfLongestLivedStars": {
-    "value": " - Lifetime of longest lived stars"
+    "value": "Lifetime of longest lived stars"
   },
   "A": {
-    "value": "<span style=\"font-family: monospace;\"><b>A</b></span>"
+    "value": "A"
   },
   "B": {
-    "value": "<span style=\"font-family: monospace;\"><b>B</b></span>"
+    "value": "B"
   },
   "C": {
-    "value": "<span style=\"font-family: monospace;\"><b>C</b></span>"
+    "value": "C"
   },
   "D": {
-    "value": "<span style=\"font-family: monospace;\"><b>D</b></span>"
+    "value": "D"
   },
   "E": {
-    "value": "<span style=\"font-family: monospace;\"><b>E</b></span>"
+    "value": "E"
   },
   "F": {
-    "value": "<span style=\"font-family: monospace;\"><b>F</b></span>"
+    "value": "F"
   },
   "G": {
-    "value": "<span style=\"font-family: monospace;\"><b>G</b></span>"
+    "value": "G"
   },
   "H": {
-    "value": "<span style=\"font-family: monospace;\"><b>H</b></span>"
+    "value": "H"
   },
   "I": {
-    "value": "<span style=\"font-family: monospace;\"><b>I</b></span>"
+    "value": "I"
   },
   "J": {
-    "value": "<span style=\"font-family: monospace;\"><b>J</b></span>"
+    "value": "J"
   },
   "availableDecays": {
     "value": "Available Decays"
@@ -158,7 +158,6 @@
   "availableDecaysInfoPanelText": {
     "value": "While other decays may exist, the available decays shown are the most common. In cases where there is no known decay, no available decays are shown."
   },
-
   "chartIntro": {
     "value": "Chart Intro"
   },
Index: js/decay/view/HalfLifeInfoDialog.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/decay/view/HalfLifeInfoDialog.ts b/js/decay/view/HalfLifeInfoDialog.ts
--- a/js/decay/view/HalfLifeInfoDialog.ts	(revision 5bf96458622725cddb04daebfb801452771b14ff)
+++ b/js/decay/view/HalfLifeInfoDialog.ts	(date 1689966316468)
@@ -9,7 +9,7 @@
 
 import Dialog from '../../../../sun/js/Dialog.js';
 import buildANucleus from '../../buildANucleus.js';
-import { HBox, Node, Rectangle, RichText, Text, VBox } from '../../../../scenery/js/imports.js';
+import { Font, GridBox, HBox, Node, Rectangle, RichText, Text, VBox } from '../../../../scenery/js/imports.js';
 import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
 import BuildANucleusStrings from '../../BuildANucleusStrings.js';
 import BANColors from '../../common/BANColors.js';
@@ -32,19 +32,19 @@
                       neutronCountProperty: TReadOnlyProperty<number>,
                       doesNuclideExistBooleanProperty: TReadOnlyProperty<boolean> ) {
 
-    const leftSideStringProperties = [
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_A_NUCLEUS.timescaleStringProperty,
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_AN_ATOM.timescaleStringProperty,
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_ONE_THOUSAND_ATOMS.timescaleStringProperty,
-      BANTimescalePoints.TIME_FOR_SOUND_TO_TRAVEL_ONE_MILLIMETER.timescaleStringProperty,
-      BANTimescalePoints.A_BLINK_OF_AN_EYE.timescaleStringProperty
+    const leftSideTimescalePoints = [
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_A_NUCLEUS,
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_AN_ATOM,
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_ONE_THOUSAND_ATOMS,
+      BANTimescalePoints.TIME_FOR_SOUND_TO_TRAVEL_ONE_MILLIMETER,
+      BANTimescalePoints.A_BLINK_OF_AN_EYE
     ];
-    const rightSideStringProperties = [
-      BANTimescalePoints.ONE_MINUTE.timescaleStringProperty,
-      BANTimescalePoints.ONE_YEAR.timescaleStringProperty,
-      BANTimescalePoints.AVERAGE_HUMAN_LIFESPAN.timescaleStringProperty,
-      BANTimescalePoints.AGE_OF_THE_UNIVERSE.timescaleStringProperty,
-      BANTimescalePoints.LIFETIME_OF_LONGEST_LIVED_STARS.timescaleStringProperty
+    const rightSideTimescalePoints = [
+      BANTimescalePoints.ONE_MINUTE,
+      BANTimescalePoints.ONE_YEAR,
+      BANTimescalePoints.AVERAGE_HUMAN_LIFESPAN,
+      BANTimescalePoints.AGE_OF_THE_UNIVERSE,
+      BANTimescalePoints.LIFETIME_OF_LONGEST_LIVED_STARS
     ];
 
     // join the strings in each array, placing one on each line
@@ -58,10 +58,41 @@
         leading: 6
       } );
     };
+
+    const createGridBox = ( timescalePoints: BANTimescalePoints[] ): Node => {
+
+      const grid = new GridBox();
+      timescalePoints.forEach( ( timescalePoint, rowIndex ) => {
+
+        grid.addRow( [ new Text( timescalePoint.timescaleMarkerStringProperty, {
+          font: new Font( {
+            size: '20px',
+            family: 'monospace',
+            weight: 'bold'
+          } ),
+          layoutOptions: {
+            minWidth: 10 // TODO:!!!!
+          }
+        } ),
+          new Text( '-', {
+            font: LEGEND_FONT,
+            layoutOptions: {
+              minWidth: 5 // TODO:!!!!
+            }
+          } ),
+          new Text( timescalePoint.timescaleDescriptionStringProperty, {
+            font: LEGEND_FONT,
+            layoutOptions: { align: 'left' }
+          } )
+        ] );
+      } );
+      return grid;
+    };
+
     const legend = new HBox( {
       children: [
-        createTextFromStringProperties( leftSideStringProperties ),
-        createTextFromStringProperties( rightSideStringProperties )
+        createGridBox( leftSideTimescalePoints )
+        // createGridBox( rightSideTimescalePoints )
       ],
       spacing: 70,
       align: 'top',
@@ -94,36 +125,23 @@
     numberLineNodeAndLegend.centerX = contents.centerX;
 
     // the half-life's of the strings, in respective order
-    const halfLifeTime = [
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_A_NUCLEUS.numberOfSeconds,
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_AN_ATOM.numberOfSeconds,
-      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_ONE_THOUSAND_ATOMS.numberOfSeconds,
-      BANTimescalePoints.TIME_FOR_SOUND_TO_TRAVEL_ONE_MILLIMETER.numberOfSeconds,
-      BANTimescalePoints.A_BLINK_OF_AN_EYE.numberOfSeconds,
-      BANTimescalePoints.ONE_MINUTE.numberOfSeconds,
-      BANTimescalePoints.ONE_YEAR.numberOfSeconds,
-      BANTimescalePoints.AVERAGE_HUMAN_LIFESPAN.numberOfSeconds,
-      BANTimescalePoints.AGE_OF_THE_UNIVERSE.numberOfSeconds,
-      BANTimescalePoints.LIFETIME_OF_LONGEST_LIVED_STARS.numberOfSeconds
-    ];
-
-    // the labels on the half-life's, in respective order
-    const halfLifeLabels = [
-      BuildANucleusStrings.AStringProperty,
-      BuildANucleusStrings.BStringProperty,
-      BuildANucleusStrings.CStringProperty,
-      BuildANucleusStrings.DStringProperty,
-      BuildANucleusStrings.EStringProperty,
-      BuildANucleusStrings.FStringProperty,
-      BuildANucleusStrings.GStringProperty,
-      BuildANucleusStrings.HStringProperty,
-      BuildANucleusStrings.IStringProperty,
-      BuildANucleusStrings.JStringProperty
+    const timescalePoints = [
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_A_NUCLEUS,
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_AN_ATOM,
+      BANTimescalePoints.TIME_FOR_LIGHT_TO_CROSS_ONE_THOUSAND_ATOMS,
+      BANTimescalePoints.TIME_FOR_SOUND_TO_TRAVEL_ONE_MILLIMETER,
+      BANTimescalePoints.A_BLINK_OF_AN_EYE,
+      BANTimescalePoints.ONE_MINUTE,
+      BANTimescalePoints.ONE_YEAR,
+      BANTimescalePoints.AVERAGE_HUMAN_LIFESPAN,
+      BANTimescalePoints.AGE_OF_THE_UNIVERSE,
+      BANTimescalePoints.LIFETIME_OF_LONGEST_LIVED_STARS
     ];
 
     // create and add the half-life arrow and label
-    for ( let i = 0; i < halfLifeTime.length; i++ ) {
-      halfLifeNumberLineNode.addArrowAndLabel( halfLifeLabels[ i ], halfLifeTime[ i ] );
+    for ( let i = 0; i < timescalePoints.length; i++ ) {
+      const timescalePoint = timescalePoints[ i ];
+      halfLifeNumberLineNode.addArrowAndLabel( timescalePoint.timescaleMarkerStringProperty, timescalePoints[ i ].numberOfSeconds );
     }
 
     const titleNode = new Text( BuildANucleusStrings.halfLifeTimescaleStringProperty, {
Index: js/decay/model/BANTimescalePoints.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/decay/model/BANTimescalePoints.ts b/js/decay/model/BANTimescalePoints.ts
--- a/js/decay/model/BANTimescalePoints.ts	(revision 5bf96458622725cddb04daebfb801452771b14ff)
+++ b/js/decay/model/BANTimescalePoints.ts	(date 1689965261288)
@@ -11,82 +11,76 @@
 import buildANucleus from '../../buildANucleus.js';
 import BuildANucleusStrings from '../../BuildANucleusStrings.js';
 import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js';
-import PatternStringProperty from '../../../../axon/js/PatternStringProperty.js';
-import Property from '../../../../axon/js/Property.js';
 
 const SECONDS_IN_A_YEAR = 365 * 24 * 60 * 60; // 365 days x 24 hrs/day x 60 min/hr x 60 sec/min
 const TIME_FOR_LIGHT_TO_CROSS_AN_ATOM = Math.pow( 10, -19 );
 
-// TODO: i18n this pattern perhaps? or discuss further https://github.com/phetsims/build-a-nucleus/issues/90
-const patternStringProperty = new Property( '{{first}}{{second}}' );
-
 class BANTimescalePoints extends EnumerationValue {
 
   public static readonly TIME_FOR_LIGHT_TO_CROSS_A_NUCLEUS = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.AStringProperty,
-      second: BuildANucleusStrings.timeForLightToCrossANucleusStringProperty
-    } ), Math.pow( 10, -23 ) );
+    BuildANucleusStrings.AStringProperty,
+    BuildANucleusStrings.timeForLightToCrossANucleusStringProperty,
+    Math.pow( 10, -23 )
+  );
 
   public static readonly TIME_FOR_LIGHT_TO_CROSS_AN_ATOM = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.BStringProperty,
-      second: BuildANucleusStrings.timeForLightToCrossAnAtomStringProperty
-    } ), TIME_FOR_LIGHT_TO_CROSS_AN_ATOM );
+    BuildANucleusStrings.BStringProperty,
+    BuildANucleusStrings.timeForLightToCrossAnAtomStringProperty,
+    TIME_FOR_LIGHT_TO_CROSS_AN_ATOM
+  );
 
   public static readonly TIME_FOR_LIGHT_TO_CROSS_ONE_THOUSAND_ATOMS = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.CStringProperty,
-      second: BuildANucleusStrings.timeForLightToCrossOneThousandAtomsStringProperty
-    } ),
+    BuildANucleusStrings.CStringProperty,
+    BuildANucleusStrings.timeForLightToCrossOneThousandAtomsStringProperty,
     TIME_FOR_LIGHT_TO_CROSS_AN_ATOM * 1000 );
 
   public static readonly TIME_FOR_SOUND_TO_TRAVEL_ONE_MILLIMETER = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.DStringProperty,
-      second: BuildANucleusStrings.timeForSoundToTravelOneMillimeterStringProperty
-    } ), 2e-6 );
+    BuildANucleusStrings.DStringProperty,
+    BuildANucleusStrings.timeForSoundToTravelOneMillimeterStringProperty,
+    2e-6
+  );
 
   public static readonly A_BLINK_OF_AN_EYE = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.EStringProperty,
-      second: BuildANucleusStrings.aBlinkOfAnEyeStringProperty
-    } ), 1 / 3 );
+    BuildANucleusStrings.EStringProperty,
+    BuildANucleusStrings.aBlinkOfAnEyeStringProperty,
+    1 / 3
+  );
 
   public static readonly ONE_MINUTE = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.FStringProperty,
-      second: BuildANucleusStrings.oneMinuteStringProperty
-    } ), 60 );
+    BuildANucleusStrings.FStringProperty,
+    BuildANucleusStrings.oneMinuteStringProperty,
+    60
+  );
 
   public static readonly ONE_YEAR = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.GStringProperty,
-      second: BuildANucleusStrings.oneYearStringProperty
-    } ), SECONDS_IN_A_YEAR );
+    BuildANucleusStrings.GStringProperty,
+    BuildANucleusStrings.oneYearStringProperty,
+    SECONDS_IN_A_YEAR
+  );
 
   public static readonly AVERAGE_HUMAN_LIFESPAN = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.HStringProperty,
-      second: BuildANucleusStrings.averageHumanLifespanStringProperty
-    } ), 72.6 * SECONDS_IN_A_YEAR );
+    BuildANucleusStrings.HStringProperty,
+    BuildANucleusStrings.averageHumanLifespanStringProperty,
+    72.6 * SECONDS_IN_A_YEAR
+  );
 
   public static readonly AGE_OF_THE_UNIVERSE = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.IStringProperty,
-      second: BuildANucleusStrings.ageOfTheUniverseStringProperty
-    } ), 13.77e9 * SECONDS_IN_A_YEAR );
+    BuildANucleusStrings.IStringProperty,
+    BuildANucleusStrings.ageOfTheUniverseStringProperty,
+    13.77e9 * SECONDS_IN_A_YEAR
+  );
 
   public static readonly LIFETIME_OF_LONGEST_LIVED_STARS = new BANTimescalePoints(
-    new PatternStringProperty( patternStringProperty, {
-      first: BuildANucleusStrings.JStringProperty,
-      second: BuildANucleusStrings.lifetimeOfLongestLivedStarsStringProperty
-    } ), 450e18 );
+    BuildANucleusStrings.JStringProperty,
+    BuildANucleusStrings.lifetimeOfLongestLivedStarsStringProperty,
+    450e18
+  );
 
   public static readonly enumeration = new Enumeration( BANTimescalePoints );
 
   public constructor(
-    public readonly timescaleStringProperty: TReadOnlyProperty<string>,
+    public readonly timescaleMarkerStringProperty: TReadOnlyProperty<string>,
+    public readonly timescaleDescriptionStringProperty: TReadOnlyProperty<string>,
     public readonly numberOfSeconds: number
   ) {
     super();
Index: js/decay/view/HalfLifeInformationNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/decay/view/HalfLifeInformationNode.ts b/js/decay/view/HalfLifeInformationNode.ts
--- a/js/decay/view/HalfLifeInformationNode.ts	(revision 5bf96458622725cddb04daebfb801452771b14ff)
+++ b/js/decay/view/HalfLifeInformationNode.ts	(date 1689966045769)
@@ -25,10 +25,10 @@
 class HalfLifeInformationNode extends Node {
 
   public constructor( halfLifeNumberProperty: TReadOnlyProperty<number>,
-               isStableBooleanProperty: TReadOnlyProperty<boolean>,
-               protonCountProperty: TReadOnlyProperty<number>,
-               neutronCountProperty: TReadOnlyProperty<number>,
-               doesNuclideExistBooleanProperty: TReadOnlyProperty<boolean> ) {
+                      isStableBooleanProperty: TReadOnlyProperty<boolean>,
+                      protonCountProperty: TReadOnlyProperty<number>,
+                      neutronCountProperty: TReadOnlyProperty<number>,
+                      doesNuclideExistBooleanProperty: TReadOnlyProperty<boolean> ) {
     super();
 
     // create and add the halfLifeNumberLineNode
@@ -46,6 +46,10 @@
     const halfLifeInfoDialog = new HalfLifeInfoDialog( halfLifeNumberProperty, isStableBooleanProperty,
       protonCountProperty, neutronCountProperty, doesNuclideExistBooleanProperty );
 
+
+    phet.joist.sim.isConstructionCompleteProperty.link( complete => {
+      complete && halfLifeInfoDialog.show();
+    } );
     // create and add the info button
     const infoButton = new InfoButton( {
       listener: () => halfLifeInfoDialog.show(),

@zepumph
Copy link
Member

zepumph commented Jul 21, 2023

I got to a commit point, refactoring things to match exactly, but instead using grid box. I have some concerns:

  • Changing many english values of sting keys means that we likely want to change them in babel right before publication.
  • Do we actually want the monospace font family now? Or could we just use a bold version of phet font?

@zepumph
Copy link
Member

zepumph commented Aug 3, 2023

We will keep the monospace font because it is the easiest solution to the jumbled-ness that occurs below without it:

image

We will work on fixing the translations too.

@zepumph
Copy link
Member

zepumph commented Aug 4, 2023

Ok. So we want to create a script that will basically convert all translations from something like:

"value": "<span style=\"font-family: monospace;\"><b>A</b></span>"

to A.

Here is a script:

// Copyright 2022, University of Colorado Boulder

/* eslint-env node */

// imports
const fs = require( 'fs' );
const path = require( 'path' );

const incorrectLetterStrings = [
  'A',
  'B',
  'C',
  'D',
  'E',
  'F',
  'G',
  'H',
  'I',
  'J'
];

const problematicPrefix = '<span style="font-family: monospace;"><b>';
const problematicSuffix = '</b></span>';


const translatedFiles = fs.readdirSync( '../babel/build-a-nucleus/', 'utf8' );//.slice( 0, 1 );

translatedFiles.forEach( filename => {
  const filenameParts = filename.replace( '.json', '' ).split( '_' );
  const locale = filenameParts.slice( 1 ).join( '' );
  const babelContents = JSON.parse( fs.readFileSync( path.join( '../babel/build-a-nucleus', filename ), 'utf-8' ) );
  incorrectLetterStrings.forEach( letter => {
    if ( babelContents[ letter ] ) {
      const noStyle = babelContents[ letter ].value.replace( problematicPrefix, '' ).replace( problematicSuffix, '' );
      if ( noStyle !== letter ) {
        console.log( locale, noStyle );
      }
    }
  } );
} );

And here is the output. It will only output situations in which the translation wasn't identical to the input (`"<span style="font-family: monospace;">C"). These we may need to handle manually.

am BB
am CC
am DD
am EE
am FF
am GG
am HH
am II
am JJ
arMA E	
ha <span style="font-family: monospace;"><b>A
is A 
is B 
is C 
is D 
is E 
is F 
is G 
is H 
is I 
is J 
ny <span style="font-family: monospace;">><b>A
om AA
om BB
om CC
om DD
om EE
om FF
om GG
om HH
om II
om JJ
pt G
ru А
ru Б
ru В
ru Г
ru Д
ru Е
ru Ж
ru З
ru И
ru К
sl Č
sl D
sl E
sl F
sl G
sl H
sl I
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>A</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>B</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>C</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>D</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>E</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>F</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>G</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>H</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>I</b></Muda>
sw <Mtindo muda="Asili-familia: nafasi moja;"><b>J</b></Muda>
uk А
uk Б
uk В
uk Г
uk Ґ
uk Д
uk Е
uk Є
uk Ж
uk З

Questions:

REMEMBER THAT WE ABSOLUTELY DO NOT WANT TO DO THIS UNTIL RIGHT BEFORE PUBLICATION.

@zepumph
Copy link
Member

zepumph commented Aug 17, 2023

Updated script that is parsing out all the weird cases so we can provide the right value for translations.

// Copyright 2022, University of Colorado Boulder

/* eslint-env node */

// imports
const fs = require( 'fs' );
const path = require( 'path' );

const incorrectLetterStrings = [
  'A',
  'B',
  'C',
  'D',
  'E',
  'F',
  'G',
  'H',
  'I',
  'J'
];

const problematicPrefix = '<span style="font-family: monospace;"><b>';
const problematicSuffix = '</b></span>';

const swTranslatedProblematicPrefix = '<Mtindo muda="Asili-familia: nafasi moja;"><b>'
const swTranslatedProblematicSuffix = '</b></Muda>';


const translatedFiles = fs.readdirSync( '../babel/build-a-nucleus/', 'utf8' );//.slice( 0, 1 );

translatedFiles.forEach( filename => {
  const filenameParts = filename.replace( '.json', '' ).split( '_' );
  const locale = filenameParts.slice( 1 ).join( '' );
  const babelContents = JSON.parse( fs.readFileSync( path.join( '../babel/build-a-nucleus', filename ), 'utf-8' ) );
  incorrectLetterStrings.forEach( letter => {
    if ( babelContents[ letter ] ) {
      const noStyle = babelContents[ letter ].value.replace( problematicPrefix, '' ).replace( problematicSuffix, '' );
      /*
      doc time:
      is: space after the letter (just trim it)
       */
      if ( noStyle.trim() === letter ) { // Space after letter, for locale `is`
        // console.log( noStyle === letter, `hi${noStyle[ 0 ]}hi${noStyle[ 1 ]}hi` );
      }
      else if ( noStyle === `${letter}${letter}` ) {
        // This applies to am and om locales
        // console.log( 'AHHHH', noStyle );
      }
      else if ( incorrectLetterStrings.includes( noStyle ) && noStyle !== letter ) {
        // sl and pt both incorrectly translated wrong characters, seems like a bug to me. Ask JB!!!
        /*
        console.log( 'AAAH', locale, noStyle, letter );
        >>>
        AAAH pt G H
        AAAH sl D E
        AAAH sl E F
        AAAH sl F G
        AAAH sl G H
        AAAH sl H I
        AAAH sl I J
         */
      }
      else if ( noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' ) === letter ) {
        // For sw, the html was translated. Get that outta there!
        // correctLetter = noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' );
        // console.log( locale, noStyle, noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' ), letter );
      }
      else if ( noStyle !== letter ) {

        // The rest are actually translated letter, and should be presevered
        // Cyrrlic А looks like A, but isn't
        console.log( locale, noStyle, noStyle.charCodeAt( 0 ), letter, letter.charCodeAt( 0 ) );
      }
    }
  } );
} );

@zepumph
Copy link
Member

zepumph commented Aug 17, 2023

Ok, I'm linking #125 into this issue, which includes fixing the dashes which are no longer in i18n strings. The commit that broke translations and needs patching is 8cae8de.

Here is a script that is pretty much complete at fixing both problems:

// Copyright 2022, University of Colorado Boulder

/* eslint-env node */

// imports
const fs = require( 'fs' );
const path = require( 'path' );

const incorrectLetterStrings = [
  'A',
  'B',
  'C',
  'D',
  'E',
  'F',
  'G',
  'H',
  'I',
  'J'
];

const problematicPrefix = '<span style="font-family: monospace;"><b>';
const problematicSuffix = '</b></span>';

const swTranslatedProblematicPrefix = '<Mtindo muda="Asili-familia: nafasi moja;"><b>';
const swTranslatedProblematicSuffix = '</b></Muda>';


const translatedFiles = fs.readdirSync( '../babel/build-a-nucleus/', 'utf8' );//.slice( 0, 1 );

const handleLetters = ( babelContents, locale ) => {
  incorrectLetterStrings.forEach( letter => {
    if ( babelContents[ letter ] ) {
      const noStyle = babelContents[ letter ].value.replace( problematicPrefix, '' ).replace( problematicSuffix, '' );
      /*
      doc time:
      is: space after the letter (just trim it)
       */
      if ( noStyle.trim() === letter ) { // Space after letter, for locale `is`
        // console.log( noStyle === letter, `hi${noStyle[ 0 ]}hi${noStyle[ 1 ]}hi` );
        babelContents[ letter ].value = letter;
      }
      else if ( noStyle === `${letter}${letter}` ) {
        // This applies to am and om locales
        // console.log( 'AHHHH', noStyle );
        babelContents[ letter ].value = letter;
      }
      else if ( incorrectLetterStrings.includes( noStyle ) && noStyle !== letter ) {
        // sl and pt both incorrectly translated wrong characters, seems like a bug to me. Ask JB!!!
        /*
        console.log( 'AAAH', locale, noStyle, letter );
        >>>
        AAAH pt G H
        AAAH sl D E
        AAAH sl E F
        AAAH sl F G
        AAAH sl G H
        AAAH sl H I
        AAAH sl I J
         */
        babelContents[ letter ].value = letter;
      }
      else if ( noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' ) === letter ) {
        // For sw, the html was translated. Get that outta there!
        // correctLetter = noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' );
        // console.log( locale, noStyle, noStyle.replace( swTranslatedProblematicPrefix, '' ).replace( swTranslatedProblematicSuffix, '' ), letter );
        babelContents[ letter ].value = letter;
      }
      else if ( noStyle !== letter ) {

        // The rest are actually translated letter, and should be presevered
        // Cyrrlic А looks like A, but isn't
        babelContents[ letter ].value = noStyle;
        // console.log( 'actual letter translation', locale, noStyle, letter ); // TODO: double check for bad cases here! https://github.com/phetsims/build-a-nucleus/issues/76
      }
      else {
        // The above should cover all cases, but log here just in case
        console.log( 'should not be here', locale, noStyle, letter ); // We should never get here!!
      }
    }
  } );
};

const keysWithDashes = [
  'timeForLightToCrossANucleus',
  'timeForLightToCrossAnAtom',
  'timeForLightToCrossOneThousandAtoms',
  'timeForSoundToTravelOneMillimeter',
  'aBlinkOfAnEye',
  'oneMinute',
  'oneYear',
  'averageHumanLifespan',
  'ageOfTheUniverse',
  'lifetimeOfLongestLivedStars'
];
const handleDashes = ( babelContents, locale ) => {
  keysWithDashes.forEach( key => {
    if ( babelContents[ key ] ) {
      const value = babelContents[ key ].value.trim();
      let newValue = value;
      if ( value.startsWith( '-' ) ) {
        newValue = value.slice( 1 ).trim();
      }
      else if ( value.endsWith( '-' ) ) {
        newValue = value.slice( 0, value.length - 1 ).trim();
      }

      if ( newValue.includes( '-' ) ) {
        // TODO: Check this out and make sure that usages of dashes here are not buggy! https://github.com/phetsims/build-a-nucleus/issues/76
        // console.log( 'dash-cover', locale, newValue );
      }
      babelContents[ key ].value = newValue;
    }
  } );
};

translatedFiles.forEach( filename => {
  const filenameParts = filename.replace( '.json', '' ).split( '_' );
  const locale = filenameParts.slice( 1 ).join( '' );
  const localeFilename = path.join( '../babel/build-a-nucleus', filename );
  const babelContents = JSON.parse( fs.readFileSync( localeFilename, 'utf-8' ) );

  handleLetters( babelContents, locale );
  handleDashes( babelContents, locale );
  fs.writeFileSync( localeFilename, JSON.stringify( babelContents, null, 2 ) );
} );

Steps once we are ready to publish:

  1. Test to make sure that this is working well still. That means doing the console logs at the end of the dashes and letters functions to make sure that newly different and buggy translations didn't come in between now and publication.
  2. Run it
  3. Revert the unicode-specific changes
  4. commit it.
  5. Then we are ready to publish.

zepumph added a commit to phetsims/babel that referenced this issue Aug 18, 2023
@zepumph
Copy link
Member

zepumph commented Aug 18, 2023

I reviewed the above with @solaolateju and we both agree with the path forward.

@zepumph
Copy link
Member

zepumph commented Aug 23, 2023

  • one thing before continuing. The script above changes the unicode string ‪‪\u202a into the actually rendered character. I don't think we want to do this, so I need to look at babel to see how we keep the unicode language in the babel repo.

After discussing with JB, I'll just revert that trouble before continuing.

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Sep 13, 2023

Noting that this issue (I think 😂) is present in phetsims/qa#977

Screenshot 2023-09-13 at 7 52 53 PM

@Nancy-Salpepi
Copy link

tagging phetsims/qa#988 to keep this on your radar 📡.

@zepumph
Copy link
Member

zepumph commented Oct 17, 2023

Alright we are ready to do this, pushed above. Proceeding to publication.

@zepumph zepumph closed this as completed Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:blocks-sim-publication type:question Further information is requested
Projects
None yet
Development

No branches or pull requests

6 participants