Studio: Translate "About Studio" modal #228

katinthehatsite · 2024-06-10T11:21:32Z

Closes https://github.com/Automattic/dotcom-forge/issues/7522

Proposed Changes

This PR makes the strings in the "About Studio" window translatable.

Testing Instructions

Pull the changes from this branch locally
Before starting the app, modify the language in locale.ts in getPreferredSystemLanguages to return French:

export function getPreferredSystemLanguages() {
	if ( process.platform === 'linux' && process.env.NODE_ENV !== 'test' ) {
			.getPreferredSystemLanguages()
			.filter( ( lang ) => supportedLocales.includes( lang ) );
	}
	return [ 'fr' ];
}

Navigate to src/translations/studio-fr.jed.json
Add the following strings and translations to the file:

            "Demo sites powered by": [
                "Sites de démonstration propulsés par"
            ],
            "Local sites powered by": [
                "Sites locaux propulsés par"
            ],
            "Share Feedback": [
                "Partager des commentaires"
            ],
            "Studio by WordPress.com": [
                "Studio par WordPress.com"
            ],
            "Version": [
                "Version"
            ]

Start the app with nvm use && npm install && npm start
Open the About Studio window
Confirm that the strings are translated to French:

Capture d’écran, le 2024-06-10 à 13 46 09

Notes:

I explored refactoring the HTML file into a React component and then rendering it into this custom window but the solution required modifying the app setup in multiple places. I opted for a more straightforward approach considering that that window is fairly static

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

katinthehatsite · 2024-06-10T11:57:26Z

src/about-menu/about-menu.html

-        <p class="studio-name">Studio by WordPress.com</p>
-        <p class="version">Version <span id="version"></span></p>
+        <p class="studio-name" id="studio-by-wpcom">Studio by WordPress.com</p>
+        <p class="version"><span id="version-text">Version</span> <span id="version"></span></p>
        <p class="links">
          <a href="https://github.com/Automattic/studio" target="_blank">GitHub</a>


I did not add translation for GitHub since it is a proper name. Happy to add it if it is considered necessary.

sejas

I've followed the testing instructions and it works as expected.

I wanted to recommend using esc_attr__ , but it's not part of @wordpress/i18n.
I suggest adding Sentry or sanitizing the strings with a custom function. My concern is that some translation would include a single quote: ' , which would break the string replacements.

sejas · 2024-06-10T22:03:16Z

src/about-menu/open-about-menu.ts

+					document.getElementById('share-feedback').innerText = '${ __( 'Share Feedback' ) }';
+					document.getElementById('demo-sites').innerText = '${ __( 'Demo sites powered by' ) }';
+					document.getElementById('local-sites').innerText = '${ __( 'Local sites powered by' ) }';`
+				)
 				.catch( ( err ) => {
 					console.error( 'Error executing JavaScript:', err );


Should we capture the error in sentry?

Added in 343ac4d

sejas · 2024-06-10T22:12:04Z

src/about-menu/open-about-menu.ts

+					`document.getElementById('version').innerText = '${ packageJson }'; 
+					document.getElementById('studio-by-wpcom').innerText = '${ __( 'Studio by WordPress.com' ) }';
+					document.getElementById('version-text').innerText = '${ __( 'Version' ) }'; 


Let's unify the word Version and the actual number. Something like sprintf( __( 'Version %s' ), packageJson )

Added in af02dbb

sejas · 2024-06-10T22:13:27Z

src/about-menu/about-menu.html

-        <p class="studio-name">Studio by WordPress.com</p>
-        <p class="version">Version <span id="version"></span></p>
+        <p class="studio-name" id="studio-by-wpcom">Studio by WordPress.com</p>
+        <p class="version"><span id="version-text">Version</span> <span id="version"></span></p>


Let's use only one string here, so RTL languages will work better.

Suggested change

Version 

Version x.y.z

Added in af02dbb

katinthehatsite · 2024-06-11T07:57:03Z

I suggest adding Sentry or sanitizing the strings with a custom function. My concern is that some translation would include a single quote: ' , which would break the string replacements.

Yeah I think you are right, I tried to break it and it did break. Let me conjure something for this. I will re-request review once I am ready

katinthehatsite · 2024-06-11T09:57:29Z

@sejas I added something simple in 6c1f64f to handle the ' to ensure that it is rendered.

I also found that we have something like https://www.npmjs.com/package/@wordpress/escape-html that we already have available and installed but a single quote breaks when I try to use escapeAtrribute or escapeHMTL. Do you think the fix about would be sufficient for this case in combination with Sentry capture?

sejas · 2024-06-11T12:53:07Z

src/about-menu/open-about-menu.ts

+	function escapeSingleQuotes( str: string ) {
+		return str.replace( /'/g, "\\'" );
+	}


Thanks for the changes!

Nice! I tested it and it renders the single quote and it doesn't break the About page.
I tried replacing to ", but it didn't render the correct entity.

I've also seen that @wordpress/escape-html escapeAttribute Only escapes, the >, " and & symbols.

wojtekn

I explored refactoring the HTML file into a React component and then rendering it into this custom window but the solution required modifying the app setup in multiple places. I opted for a more straightforward approach considering that that window is fairly static

Thanks for sharing the details explaining the approach you took. It seems reasonable to do so.

wojtekn · 2024-06-12T10:29:46Z

src/about-menu/about-menu.html

  </div>
  <div class="info">
-    <p>Local sites powered by<br><a href="https://wordpress.org/playground/" target="_blank">WordPress Playground ↗</a></p>
+    <p><span id="local-sites">Local sites powered by</span><br><a href="https://wordpress.org/playground/" target="_blank">WordPress Playground ↗</a></p>


Do we need those phrases in two places: here in the template and in the file that replaces those with translations?

Do you mean why the translation is divided in two "divs"?
Local sites powered by <a href="https://wordpress.org/playground/" target="_blank">WordPress Playground ↗</a> instead of Local sites powered by <a href="https://wordpress.org/playground/" target="_blank">WordPress Playground ↗</a>

When reviewing I thought it could produce errors in RTL languages but the fact that we display one on top of the other solves any issue.

No, I meant that tests, e.g., Local sites powered by, are now placed in two places.

I am guessing that your question is if it could be removed from https://github.com/Automattic/studio/blob/c2c9c20219ea884493bd3ba8ffbc64f2bc5cc1ce/src/about-menu/about-menu.html#L90 since we are inserting it anyway so that it would be something like:

 <a href="https://wordpress.com/hosting/?utm_source=studio&utm_medium=referral&utm_campaign=about_screen" target="_blank">WordPress.com hosting ↗</a>

(so just the id and no text)

Is that right?

Yes, this is correct. I was wondering if, with the current implementation, there is a reason to keep it in the file template, too. Does it work as a fallback, so if the translation couldn't be loaded, we would display the English version included in the template? Or could such a situation not occur?

My point is that keeping the source text in two places comes with a risk that someone updates it in one place, skipping the latter.

Yes, that's right. The strings in the HTML file serve as fallback content. This ensures that if the JavaScript fails to inject the translations or if there is any issue with the translation strings, the user will still see the default text that is added in the HTML file.

However, you bring up an important point - it might not be obvious that the code needs to be handled in two places. I will create a PR to add clarifying comments to this code.

I created a PR here to add some clarifying comments:

#290

Thanks for following up on this and adding those comments. It will be helpful for future us!

Remove unused script

264cc2d

katinthehatsite self-assigned this Jun 10, 2024

katinthehatsite added the Studio App label Jun 10, 2024

Add and inject translatable strings

8fa1d01

katinthehatsite requested a review from a team June 10, 2024 11:53

katinthehatsite commented Jun 10, 2024

View reviewed changes

sejas approved these changes Jun 10, 2024

View reviewed changes

Kateryna Kodonenko added 3 commits June 11, 2024 09:23

Capture error in Sentry

343ac4d

Combine version and version number in one string

4dec4f8

Add sprintif

af02dbb

Replace a signle quote

6c1f64f

sejas approved these changes Jun 11, 2024

View reviewed changes

katinthehatsite merged commit 216154a into trunk Jun 11, 2024
11 checks passed

katinthehatsite deleted the add/translate-about-window branch June 11, 2024 13:52

wojtekn reviewed Jun 12, 2024

View reviewed changes

wojtekn removed the Studio App label Jun 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Studio: Translate "About Studio" modal #228

Studio: Translate "About Studio" modal #228

katinthehatsite commented Jun 10, 2024 •

edited

Loading

katinthehatsite Jun 10, 2024

sejas left a comment

sejas Jun 10, 2024

katinthehatsite Jun 11, 2024

sejas Jun 10, 2024

katinthehatsite Jun 11, 2024

sejas Jun 10, 2024

katinthehatsite Jun 11, 2024

katinthehatsite commented Jun 11, 2024

katinthehatsite commented Jun 11, 2024 •

edited

Loading

sejas Jun 11, 2024 •

edited

Loading

wojtekn left a comment

wojtekn Jun 12, 2024

sejas Jun 18, 2024

wojtekn Jun 19, 2024

katinthehatsite Jun 20, 2024

wojtekn Jun 20, 2024

katinthehatsite Jun 24, 2024

katinthehatsite Jun 24, 2024

wojtekn Jul 3, 2024

	<p class="version"><span id="version-text">Version</span> <span id="version"></span></p>
	<p class="version"><span id="version-text">Version x.y.z</span></p>

Studio: Translate "About Studio" modal #228

Studio: Translate "About Studio" modal #228

Conversation

katinthehatsite commented Jun 10, 2024 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

Choose a reason for hiding this comment

sejas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katinthehatsite commented Jun 11, 2024

katinthehatsite commented Jun 11, 2024 • edited Loading

sejas Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

wojtekn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katinthehatsite commented Jun 10, 2024 •

edited

Loading

katinthehatsite commented Jun 11, 2024 •

edited

Loading

sejas Jun 11, 2024 •

edited

Loading