feat: Add Trans component for interpolating JSX in translations #6534

bancek · 2023-04-28T16:41:25Z

Fixes #6351
This is a proof of concept PR that shows a possible solution to #6351 (this already works, you can quickly test it by applying the diff at the bottom and removing if (isBrave() && !isMeasureTextSupported()) { in App.tsx).

Trans component make it possible to use JSX for translations.

Examples:

{
  "example1": "Hello {{audience}}",
  "example2": "Please <link>click the button</link> to continue.",
  "example3": "Please <link>click {{location}}</link> to continue.",
  "example4": "Please <link>click <bold>{{location}}</bold></link> to continue.",
}

<Trans i18nKey="example1" audience="world" />

<Trans
  i18nKey="example2"
  connectLink={(el) => <a href="https://example.com">{el}</a>}
/>

<Trans
  i18nKey="example3"
  connectLink={(el) => <a href="https://example.com">{el}</a>}
  location="the button"
/>

<Trans
  i18nKey="example4"
  connectLink={(el) => <a href="https://example.com">{el}</a>}
  location="the button"
  bold={(el) => <strong>{el}</strong>}
/>

Output:

Hello world
Please <a href="https://example.com">click the button</a> to continue.
Please <a href="https://example.com">click the button</a> to continue.
Please <a href="https://example.com">click <strong>the button</strong></a> to continue.

Trans component can also be used instead of t(). E.g. <Trans i18nKey="my.key" /> is the same as {t("my.key")} and <Trans i18nKey="my.key" name={currentName} /> is the same as {t("my.key", { name: currentName })}

vercel · 2023-04-28T16:41:29Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
excalidraw	✅ Ready (Inspect)	Visit Preview	May 3, 2023 0:25am
excalidraw-package-example	✅ Ready (Inspect)	Visit Preview	May 3, 2023 0:25am

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
docs	⬜️ Ignored (Inspect)		May 3, 2023 0:25am

ad1992 · 2023-04-28T17:02:05Z

Wohoo @bancek that was super quick 🚀
Just did a quick review and its looking good!
As you spoke about a custom component, so if its similar to that one then how about creating a library out of it so its easy to maintain as well ?
cc @dwelle

bancek · 2023-04-28T17:07:31Z

@ad1992 I took the code from Trans component from our codebase but we use i18next so I addapted it to your jotai based translations. I don't think it makes much sense to make a library for ~70 lines of code.

ad1992 · 2023-04-28T17:10:47Z

@ad1992 I took the code from Trans component from our codebase but we use i18next so I addapted it to your jotai based translations. I don't think it makes much sense to make a library for ~70 lines of code.

Alright, lets keep as is in the codebase, we can figure out later if want to move all i18n related stuff to a lib.

dwelle · 2023-04-28T18:44:31Z

src/components/Trans.tsx

+const Trans = memo<{ i18nKey: string; [key: string]: any }>(
+  ({ i18nKey, children, ...props }) => {
+    const { t, langCode } = useI18n();
+
+    const transChildren = React.useMemo(() => {


The useMemo will never memoize due to props, and the component memo won't work either unless we memoized each render function pass to it which I doubt we'll do / would yield much benefit.

It depends on the type of props. Props values can also be just strings (see example 1) but I agree that in the Excalidraw codebase, Trans props will probably be more complex so memoization won't make much sense.

Removed memo and useMemo and added tests.

Props values can also be just strings (see example 1)

~~what would be the use case for using strings?~~ EDIT: nvm, the <Trans/> is meant to also be usable as t().

Yes, <Trans i18nKey="my.key" /> is the same as {t("my.key")}.

bancek · 2023-04-28T20:06:37Z

@ad1992 I've removed the commit for BraveMeasureTextError and added the diff to the PR description.

dwelle · 2023-04-28T20:22:28Z

Thanks @bancek!

One note, what's the [bold]xxx[/bold] syntax ([]) for. My current understanding it's a DSL you guys use in your own codebase that's further transformed, and as such nothing we would be using ourselves, correct?

bancek · 2023-04-28T20:28:29Z

I would have to update the examples in the PR description. I just copied those from our internal tests to showcase what is possible. The tests in this PR render proper HTML.

bancek · 2023-04-28T23:04:42Z

I've updated the examples. Now it should be more obvious what <Trans /> does.

src/components/Trans.tsx

bancek · 2023-05-01T08:26:01Z

@ad1992 thanks for adding the comments ❤️

ad1992 · 2023-05-01T16:11:55Z

Hey so as we we were discussing in chat with @dwelle
Should we be changing the translation string api a bit, instead of appending start and end to the strings, can we use tags instead ?
Eg

"example2": "Please <link>click <bold>{{location}}</bold></link>} to continue.",
<Trans
  i18nKey="example2"
  link={(el: any) => <a href="https://example.com">{el}</a>}
  location="the button"
  bold={(el: any) => <strong>{el}</strong>}
/>

So Trans API remains the same, the translation string uses tags instead of start/end strings for better readability in case of non string values.

dwelle · 2023-05-01T16:47:22Z

If we don't want to use <tag></tag> syntax, e.g. because it evokes HTML/JSX, we can use Mustache-inspired syntax {{tag}}{{/tag}}, or Mustache syntax directly {{#tag}}{{/tag}}. In fact the latter may be preferable due to point (1) below.

The reason for switch the syntax:

right now the open/close tags look like regular variables: hard to read, and potentially conflicting with variable names
harder to edit/select in editor (you can't select the opening tag to highlight/find the closing tag)

bancek · 2023-05-01T16:51:40Z

I've implemented support for Please <link>click <bold>{{location}}</bold></link>} to continue.. It's a bit more code (it can probably be cleaned up a bit) but the translations strings are now much cleaner indeed.

ad1992 · 2023-05-01T19:10:23Z

If we don't want to use <tag></tag> syntax, e.g. because it evokes HTML/JSX, we can use Mustache-inspired syntax {{tag}}{{/tag}}, or Mustache syntax directly {{#tag}}{{/tag}}. In fact the latter may be preferable due to point (1) below.

The reason for switch the syntax:

right now the open/close tags look like regular variables: hard to read, and potentially conflicting with variable names

harder to edit/select in editor (you can't select the opening tag to highlight/find the closing tag)

I think <tag></tag> would be better coz ultimately on DOM they are replaced with some html tags so by looking at translate string it would be conveyed that these tags are replaced with some valid html tag on actual string. As per the naming thats up to us so can make sure the naming is done correctly and closer to html tags so it conveys the same meaning too.

ad1992 · 2023-05-02T06:18:40Z

src/components/Trans.tsx

+    },
+  ];
+
+  format.split(REGEXP).forEach((match) => {


the split here could be confusing as its introducing unncessary undefined tags so I think its better if we improve the split here so tokens are correct
eg for "Hello {{audience}}" its splitting to

[ 'Hello ', '{{audience}}', undefined, undefined, '' ]

so we should remove those undefined tokens

I don't know if it's possible to write a regex with delimiters in JavaScript so that it won't produce undefined results. For cleaner code we could use format.split(REGEXP).filter(Boolean) which will filter our empty strings and undefined.

yes I meant filtering only after tokens are generated

ad1992 · 2023-05-02T06:19:50Z

src/components/Trans.tsx

+        );
+      }
+
+      return;


this shouldn't be needed if we use else If instead of multiple ifs and readability wise will be better as well

It's a compromise between readability and performance. If you want to use else if you need to generate all regex matches before.

const tagStartMatch = match.match(TAG_START_REGEXP); const tagEndMatch = match.match(TAG_END_REGEXP); const keyMatch = match.match(KEY_REGEXP); if (tagStartMatch !== null) { } else if (tagEndMatch != null) { } else if (keyMatch !== null) { } else { }

src/components/Trans.tsx

ad1992 · 2023-05-02T06:29:35Z

src/components/Trans.tsx

+
+import { useI18n } from "../i18n";
+
+const REGEXP = /({{\w+?}})|(<\w+?>)|(<\/\w+?>)/g;


lets add a comment for this regex to be more clear

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>

bancek · 2023-05-02T08:41:30Z

@ad1992 I'll be offline for the rest of the day so you can fix things yourself or I can do it tomorrow.

ad1992 · 2023-05-05T16:05:10Z

Merging 🚀 Thanks @bancek ❤️

bancek · 2023-05-05T16:43:01Z

src/locales/en.json

+      "line1": "Looks like you are using Brave browser with the <bold>Aggressively Block Fingerprinting</bold> setting enabled.",
+      "line2": "This could result in breaking the <bold>Text Elements</bold> in your drawings.",
+      "line3": "We strongly recommend disabling this setting. You can follow <link>these steps</link> on how to do so.",
+      "line4": " If disabling this setting doesn't fix the display of text elements, please open an <issueLink>issue</issueLink> on our GitHub, or write us on <discordLink>Discord</discordLink>",


There is a leading space in line4 and discord is not needed anymore.

thanks for the catch. Fixed in #6561

bancek · 2023-05-05T16:43:58Z

This is so much better now. Thanks for your help @ad1992

…lidraw#6534) * feat: add Trans component * Add comments * tweak * Move brave to trans component * fix test and tweaks * remove any * fix * fix * comment * replace render function type * Use tags for Trans * Fix a typo Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com> * Cleanup, add comments, add support for kebab case * tweaks --------- Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com> Co-authored-by: dwelle <luzar.david@gmail.com>

bancek mentioned this pull request Apr 28, 2023

strings: formatting makes strings barely translatable #6351

Closed

vercel bot deployed to Preview – excalidraw-package-example April 28, 2023 16:42 View deployment

vercel bot deployed to Preview – excalidraw April 28, 2023 16:42 View deployment

dwelle reviewed Apr 28, 2023

View reviewed changes

feat: add Trans component

c28934a

bancek force-pushed the trans-component branch from 25e6fcb to c28934a Compare April 28, 2023 20:04

vercel bot deployed to Preview – excalidraw-package-example April 28, 2023 20:04 View deployment

vercel bot deployed to Preview – excalidraw April 28, 2023 20:05 View deployment

bancek changed the title ~~poc: Trans component~~ feat: Add Trans component Apr 28, 2023

Add comments

da16e06

vercel bot deployed to Preview – excalidraw-package-example May 1, 2023 08:19 View deployment

vercel bot deployed to Preview – excalidraw May 1, 2023 08:19 View deployment

tweak

1c17ef2

vercel bot deployed to Preview – excalidraw-package-example May 1, 2023 08:22 View deployment

vercel bot deployed to Preview – excalidraw May 1, 2023 08:22 View deployment

bancek commented May 1, 2023

View reviewed changes

src/components/Trans.tsx Outdated Show resolved Hide resolved

bancek commented May 1, 2023

View reviewed changes

src/components/Trans.tsx Outdated Show resolved Hide resolved

bancek commented May 1, 2023

View reviewed changes

src/components/Trans.tsx Outdated Show resolved Hide resolved

Move brave to trans component

173c8f7

vercel bot deployed to Preview – excalidraw-package-example May 1, 2023 08:38 View deployment

vercel bot deployed to Preview – excalidraw May 1, 2023 08:38 View deployment

replace render function type

58cbd2f

vercel bot deployed to Preview – excalidraw-package-example May 1, 2023 14:56 View deployment

vercel bot deployed to Preview – excalidraw May 1, 2023 14:56 View deployment

Use tags for Trans

ff597bd

vercel bot deployed to Preview – excalidraw-package-example May 1, 2023 16:51 View deployment

vercel bot deployed to Preview – excalidraw May 1, 2023 16:51 View deployment

ad1992 reviewed May 2, 2023

View reviewed changes

Fix a typo

4a059fa

Co-authored-by: Aakansha Doshi <aakansha1216@gmail.com>

vercel bot deployed to Preview – excalidraw-package-example May 2, 2023 08:40 View deployment

vercel bot deployed to Preview – excalidraw May 2, 2023 08:41 View deployment

Cleanup, add comments, add support for kebab case

d641179

vercel bot deployed to Preview – excalidraw-package-example May 3, 2023 07:39 View deployment

vercel bot deployed to Preview – excalidraw May 3, 2023 07:39 View deployment

tweaks

53d61a6

vercel bot deployed to Preview – excalidraw-package-example May 3, 2023 12:24 View deployment

vercel bot deployed to Preview – excalidraw May 3, 2023 12:25 View deployment

ad1992 approved these changes May 3, 2023

View reviewed changes

ad1992 merged commit 1184a8c into excalidraw:master May 5, 2023
4 checks passed

bancek commented May 5, 2023

View reviewed changes

Contextualist mentioned this pull request May 6, 2023

fix: i18n: Apply Trans component to publish library dialogue #6564

Merged

This was referenced Nov 15, 2023

feat/v0.16.1/env dynamic in docker container alswl/excalidraw#10

Closed

feat/v0.16.1/add basic http backend alswl/excalidraw#12

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Trans component for interpolating JSX in translations #6534

feat: Add Trans component for interpolating JSX in translations #6534

bancek commented Apr 28, 2023 •

edited

Loading

vercel bot commented Apr 28, 2023 •

edited

Loading

ad1992 commented Apr 28, 2023

bancek commented Apr 28, 2023

ad1992 commented Apr 28, 2023

dwelle Apr 28, 2023

bancek Apr 28, 2023

bancek Apr 28, 2023

dwelle Apr 28, 2023 •

edited

Loading

bancek Apr 28, 2023

bancek commented Apr 28, 2023

dwelle commented Apr 28, 2023

bancek commented Apr 28, 2023

bancek commented Apr 28, 2023

bancek commented May 1, 2023

ad1992 commented May 1, 2023

dwelle commented May 1, 2023 •

edited

Loading

bancek commented May 1, 2023

ad1992 commented May 1, 2023 •

edited

Loading

ad1992 May 2, 2023

bancek May 2, 2023

ad1992 May 2, 2023

ad1992 May 2, 2023

bancek May 2, 2023

ad1992 May 2, 2023

bancek commented May 2, 2023

ad1992 commented May 5, 2023

bancek May 5, 2023

dwelle May 5, 2023

bancek commented May 5, 2023

+                      );
+                    }
+                    return;


		import { useI18n } from "../i18n";

		const REGEXP = /({{\w+?}})\|(<\w+?>)\|(<\/\w+?>)/g;

feat: Add Trans component for interpolating JSX in translations #6534

feat: Add Trans component for interpolating JSX in translations #6534

Conversation

bancek commented Apr 28, 2023 • edited Loading

vercel bot commented Apr 28, 2023 • edited Loading

ad1992 commented Apr 28, 2023

bancek commented Apr 28, 2023

ad1992 commented Apr 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwelle Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bancek commented Apr 28, 2023

dwelle commented Apr 28, 2023

bancek commented Apr 28, 2023

bancek commented Apr 28, 2023

bancek commented May 1, 2023

ad1992 commented May 1, 2023

dwelle commented May 1, 2023 • edited Loading

bancek commented May 1, 2023

ad1992 commented May 1, 2023 • edited Loading

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

bancek commented May 2, 2023

ad1992 commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bancek commented May 5, 2023

bancek commented Apr 28, 2023 •

edited

Loading

vercel bot commented Apr 28, 2023 •

edited

Loading

dwelle Apr 28, 2023 •

edited

Loading

dwelle commented May 1, 2023 •

edited

Loading

ad1992 commented May 1, 2023 •

edited

Loading