add MetaMaskTranslation component by brad-decker · Pull Request #10405 · MetaMask/metamask-extension

brad-decker · 2021-02-08T23:15:13Z

Rationale

The template methods of the future addEthereumChain confirmation UI are passed in the t function for i18n strings. The problem lies in special cases where t requires JSX replacements, such as embedding a link in a string of text. To do this in normal situations you'd simply call:

content: t('yourKey', [<a>t('linkKey')</a>])

This could be done in the current setup, however, the result would require React to be in scope with the template files which I was trying to avoid. It'd also end up embedding the resulting string with jsx into the template object which seems like it could be prone to memory leaks.

This solution adds a new MetaMaskTranslation component that can render the MetaMask template objects. Additional checks are performed to ensure a maximum depth.

content: {
  element: 'MetaMaskTranslation',
  key: 'thekey',
  props: {
    translationKey: 'yourKey',
    variables: [{
      element: 'a',
      key: 'link',
      children: {
        element: 'MetaMaskTranslation',
        props: {
          translationKey: 'linkKey',
        }
      }
   }]
  }
}

To provide a path forward for requiring t to only be called with string literals, I have proposed some changes to the verify-locale script that would check for either t('$1') or translationKey: '$1' when determining if keys had been used. It also provides a mechanism for gradually switching verify-locales over to use the more strict search.

metamaskbot · 2021-02-08T23:22:23Z

Builds ready [14cd336]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (576 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	46	92	61	12	6
		domContentLoaded	398	652	574	59	29
		load	399	653	576	60	29
		domInteractive	397	651	574	59	29

kumavis · 2021-02-09T00:20:45Z

i tend to not camelcase metamask in code, so id prefer MetamaskTranslation. will defer to whatever has already been standardized

kumavis · 2021-02-09T00:22:19Z

ui/app/components/app/metamask-template-renderer/safe-component-list.js

 import Box from '../../ui/box';
+import MetaMaskTranslation from '../metamask-translation';

 export const safeComponentList = {


what is this list? what does "safe" mean here and how do we use the list

the word safe in this variable name is sort of an aspirational word. In its current iteration, it acts as the map from the element key in the template language to a real component. It is an attenuation point from 'every HTML element and component in the application' to a small number of components that we want to support in the template system.

In the future, i'd like to extend this implementation to also providing 'allow/deny' lists for props and prop validation. This way we could "safely" allow third parties to add templates with 'a' tags or 'button' where we can isolate the types of actions that can happen on those click handlers.

kumavis · 2021-02-09T00:27:01Z

can we get a storybook usage example 😁

brad-decker · 2021-02-09T15:28:13Z

can we get a storybook usage example 😁

Absolutely, @kumavis!

brad-decker · 2021-02-09T18:54:01Z

Added a very mindful and present story. (default input with da selected in language selector) (language selector is awesome btw :D 🎉 )

metamaskbot · 2021-02-09T19:08:01Z

Builds ready [3a859a3]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (609 ± 34 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	52	87	62	8	4
		domContentLoaded	403	794	608	70	34
		load	404	795	609	70	34
		domInteractive	403	793	607	70	34

brad-decker · 2021-02-10T00:15:22Z

i tend to not camelcase metamask in code, so id prefer MetamaskTranslation. will defer to whatever has already been standardized

MetaMaskTemplateRenderer landed ahead of this. I'll do a follow pr to change both

development/verify-locale-strings.js

Gudahtt · 2021-02-11T17:44:51Z

development/verify-locale-strings.js

+  });
+  const javascriptFilesToStrictSearch = await glob(globsToStrictSearch);

+  const strictSearchRegex = /\bt\('(\w+)'\)|\bt\('(\W+)'\)|\btranslationKey:\s'(\w+)'|\btranslationKey:\s'(\W+)'/gu;


Phew, OK, having trouble dissecting this one.

/\bt$'(\w+)'$/

^ That makes sense. It looks for t('anykey'). Got it.

But this one:

\bt$'(\W+)'$

No idea what's going on there. Why would we want to match non-word characters in single quotes?

Also, neither of these accounts for this:

t( 'veryLongKeyThatWasPutOnASeparateLine' )

I removed the \W checks and added a 0 or more whitespace selector before and after the string to catch cases where the call is broke up onto multiple lines. For the object notation selector i added a 0 or more whitespace selector after the colon to allow for multiline checks there as well.

development/verify-locale-strings.js

ui/app/components/app/metamask-translation/metamask-translation.js

Gudahtt · 2021-02-11T21:43:58Z

ui/app/components/app/metamask-translation/metamask-translation.js

+ * to make sure that the tree has a single root node, with maximum two leaves.
+ * Each subnode can have a maximum of one child that must be a string.
+ *
+ * This enforces a maximum recursion depth of 2, preventing translation strings


We might already have a case that doesn't meet these criteria: a link with a tooltip. The tooltip wrapper would be the first component, then the link would be the child, then the link text would be the child of the link.

Maybe the template components won't work like that, I don't know. I guess we'll see, and we can always adjust this later if we need to.

We'll need to be careful not to let this become a perverse incentive, and design weird components like LinkWithTooltip to get around this.

ui/app/components/app/metamask-translation/metamask-translation.js

…n.js Co-authored-by: Mark Stacey <markjstacey@gmail.com>

…etaMask/metamask-extension into metamask-translation-component

metamaskbot · 2021-02-12T15:48:50Z

Builds ready [c640481]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
storybook: Storybook
all artifacts

Page Load Metrics (647 ± 39 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	47	97	65	12	6
		domContentLoaded	397	783	646	81	39
		load	398	784	647	81	39
		domInteractive	397	783	645	80	39

Gudahtt

LGTM, great work! The strategy for gradually phasing in strict validation for the string-literal translation keys is a fantastic idea.

add MetaMaskTranslation component

14cd336

brad-decker marked this pull request as ready for review February 8, 2021 23:22

brad-decker requested review from a team and kumavis as code owners February 8, 2021 23:22

brad-decker requested a review from Gudahtt February 8, 2021 23:22

kumavis reviewed Feb 9, 2021

View reviewed changes

add stories

3a859a3

Gudahtt reviewed Feb 11, 2021

View reviewed changes

ui/app/components/app/metamask-translation/metamask-translation.js Outdated Show resolved Hide resolved

brad-decker and others added 4 commits February 12, 2021 09:26

Update ui/app/components/app/metamask-translation/metamask-translatio…

6a55445

…n.js Co-authored-by: Mark Stacey <markjstacey@gmail.com>

Update ui/app/components/app/metamask-translation/metamask-translatio…

60f9725

…n.js Co-authored-by: Mark Stacey <markjstacey@gmail.com>

fix regex

22e4e37

Merge branch 'metamask-translation-component' of https://github.com/M…

c640481

…etaMask/metamask-extension into metamask-translation-component

Gudahtt approved these changes Feb 12, 2021

View reviewed changes

brad-decker merged commit 8e2d4e6 into develop Feb 12, 2021

brad-decker deleted the metamask-translation-component branch February 12, 2021 17:24

github-actions bot locked and limited conversation to collaborators Feb 12, 2021

Conversation

brad-decker commented Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Uh oh!

metamaskbot commented Feb 8, 2021

Uh oh!

kumavis commented Feb 9, 2021

Uh oh!

kumavis Feb 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brad-decker Feb 9, 2021

Choose a reason for hiding this comment

Uh oh!

kumavis commented Feb 9, 2021

Uh oh!

brad-decker commented Feb 9, 2021

Uh oh!

brad-decker commented Feb 9, 2021

Uh oh!

metamaskbot commented Feb 9, 2021

Uh oh!

brad-decker commented Feb 10, 2021

Uh oh!

Uh oh!

Gudahtt Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

brad-decker Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Gudahtt Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

metamaskbot commented Feb 12, 2021

Uh oh!

Gudahtt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brad-decker commented Feb 8, 2021 •

edited

Loading

kumavis Feb 9, 2021 •

edited

Loading