[CCR] i18n feedback #30028

cjcenizal · 2019-02-05T01:08:00Z

Addresses feedback from #27936 (review).

jscodeshift

I used jscodeshift to convert from intl.formatMessage to i18n.translate. I installed the tool and used this command to apply the codemod:

jscodeshift -t=path/to/intlFormatMessageToi18nTranslate.js /path/to/kibana/x-pack/plugins/cross_cluster_replication/public/app

Codemod

Here's the contents of intlFormatMessageToi18nTranslate.js:

export default function transformer(file, api) {
  const j = api.jscodeshift;
  const root = j(file.source);
  const intlCalls = root.find(j.CallExpression, {
    callee: {
      object: {
        name: 'intl'
      },
      property: {
        name: 'formatMessage'
      }
    }
  });

  intlCalls.forEach(({ node }) => {
    node.callee.object.name = 'i18n';
    node.callee.property.name = 'translate';

    const id = node.arguments[0].properties.find(prop => {
      return prop.key.name === 'id';
    }).value.value;

    const defaultMessageNode = node.arguments[0].properties.find(prop => {
      return prop.key.name === 'defaultMessage';
    });

    const defaultMessage = typeof defaultMessageNode.value.value === 'string'
      ? defaultMessageNode.value.value
      : defaultMessageNode.value.quasis[0].value.raw;

    const values = node.arguments[1];

    const args = [
      j.property(
        'init',
        j.identifier('defaultMessage'),
        j.literal(defaultMessage)
      )
    ];

    if (!!values) {
      args.push(j.property(
        'init',
        j.identifier('values'),
        values
      ));
    }

    const config = j.objectExpression(args);
    node.arguments = [`'${id}'`, config];
  });

  return root.toSource();
};

Resources

elasticmachine · 2019-02-05T01:08:02Z

Pinging @elastic/es-ui

sebelga · 2019-02-05T08:49:27Z

I would not have mixed the i18n changes requested with the huge refactor for a future deprecation of inject18n one day before FF. It is very hard to review and we have very little time to test and discover anything that might have been left. Is there a way we could have 2 PRs and only merge before FF the i18n issue and not the implementation?

I guess you double checked with @jen-huang and we don't have any opened PR on our feature branch to avoid conflict nightmare 😊

Note: don't misunderstand me, I think this jscodeshift is awesome and you did an amazing job with it (very nice resources!), just that it seems like a lot of changes at this stage.

cjcenizal · 2019-02-05T14:07:44Z

@sebelga To make the review easier to manage you can set GitHub to ignore whitespace. Here's a quick link to the diff with whitepsace ignored: https://github.com/elastic/kibana/pull/30028/files?w=1. Does this help?

For any open PRs, we'll resolve merge conflicts either on this branch or on the other -- I'll be happy to take on that responsibility. 😄

elasticmachine · 2019-02-05T23:26:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

maryia-lapata

LGTM, but I'd like to ask to replace double quotes with single quotes for defaultMessage values in i18n.translate just to be consistent.

Thank you for jscodeshift!

maryia-lapata · 2019-02-06T15:25:26Z

x-pack/plugins/cross_cluster_replication/public/app/components/auto_follow_pattern_form.js

+      const errorMsg = i18n.translate(
+        'xpack.crossClusterReplication.autoFollowPatternForm.leaderIndexPatternError.duplicateMessage',
+        {
+          defaultMessage: "Duplicate leader index pattern aren't allowed."


nit: just to be consistent

Suggested change

defaultMessage: "Duplicate leader index pattern aren't allowed."

defaultMessage: `Duplicate leader index pattern aren't allowed.`

maryia-lapata · 2019-02-06T15:26:23Z

x-pack/plugins/cross_cluster_replication/public/app/components/auto_follow_pattern_form.js

+              placeholder={i18n.translate(
+                'xpack.crossClusterReplication.autoFollowPatternForm.fieldLeaderIndexPatternsPlaceholder',
+                {
+                  defaultMessage: "Type and then hit ENTER"


nit:

Suggested change

defaultMessage: "Type and then hit ENTER"

defaultMessage: 'Type and then hit ENTER'

@mayya-sharipova on your previous comment you suggested the ` character and there the ' 😊 Which one should be use best?

maryia-lapata · 2019-02-06T15:26:53Z

...ugins/cross_cluster_replication/public/app/components/auto_follow_pattern_indices_preview.js

+  const title = i18n.translate(
+    'xpack.crossClusterReplication.autoFollowPatternForm.indicesPreviewTitle',
+    {
+      defaultMessage: "Index name examples"


nit:

Suggested change

defaultMessage: "Index name examples"

defaultMessage: 'Index name examples'

Apparently the single quote.

maryia-lapata · 2019-02-06T15:34:58Z

...ck/plugins/cross_cluster_replication/public/app/components/follower_index_resume_provider.js

@@ -18,7 +19,7 @@ import routing from '../services/routing';
 import { resumeFollowerIndex } from '../store/actions';
 import { arrify } from '../../../common/services/utils';

-class Provider extends PureComponent {
+class FollowerIndexResumeProviderUi extends PureComponent {


We used Ui (or Component) postfix to differ the initial component and the wrapper: ComponentName = injectI18n(ComponentNameUi).
Since we are getting rid of injectI18n, we can remove Ui at the end of class name.
But it's up to you, you can leave it as well.

maryia-lapata · 2019-02-06T15:36:40Z

.../plugins/cross_cluster_replication/public/app/components/follower_index_unfollow_provider.js

+      ) : i18n.translate(
+        'xpack.crossClusterReplication.unfollowLeaderIndex.confirmModal.unfollowMultipleTitle',
+        {
+          defaultMessage: `Unfollow {count} leader indices?`,


Suggested change

defaultMessage: `Unfollow {count} leader indices?`,

defaultMessage: 'Unfollow {count} leader indices?',

sebelga

LGTM! Great job on this. It was indeed much easier to review without the white spacing. 😊

sebelga · 2019-02-08T08:53:14Z

x-pack/plugins/cross_cluster_replication/public/app/components/auto_follow_pattern_form.js

+              placeholder={i18n.translate(
+                'xpack.crossClusterReplication.autoFollowPatternForm.fieldLeaderIndexPatternsPlaceholder',
+                {
+                  defaultMessage: "Type and then hit ENTER"


@mayya-sharipova on your previous comment you suggested the ` character and there the ' 😊 Which one should be use best?

sebelga · 2019-02-08T08:54:09Z

...ugins/cross_cluster_replication/public/app/components/auto_follow_pattern_indices_preview.js

+  const title = i18n.translate(
+    'xpack.crossClusterReplication.autoFollowPatternForm.indicesPreviewTitle',
+    {
+      defaultMessage: "Index name examples"


Apparently the single quote.

sebelga · 2019-02-08T08:56:48Z

...ss_cluster_replication/public/app/components/follower_index_form/advanced_settings_fields.js

-          follower index is synchronized with the leader index; when the timeout has elapsed, the
-          poll for operations will return to the follower so that it can update some statistics, and
-          then the follower will immediately attempt to read from the leader again.`
+        defaultMessage: 'The maximum time to wait for new operations on the remote cluster when the ' +


@maryia-lapata The fact that we can't use template literals for multiple lines, is it because the translators don't have the text wrapped in their translation tool? It seems like going backward for us 😊 (as template literals were brought to solve this concatenating problem)

maryia-lapata · 2019-02-08T11:32:50Z

@sebelga we added backtick strings support to avoid escaping single quotes in defaultMessage. The concatenation of static strings is preferable because backtick strings will insert a \n for every line break and this won't work in places where this \n will actually be rendered (e.g. because of using the translated string in console.log or a <pre> tag).

So the choice path is the following:

we use backtick strings in JS in case defaultMessage contains single quotes ':
```
i18n.translate('someId', { defaultMessage: `This's a label` })
```

we use single quotes in JS otherwise:

i18n.translate('someId', { defaultMessage: 'This is a label' })

we use double quotes in JSX and HTML:

<FormattedMessage id="someId" defaultMessage="This is a label"/>

<span i18n-id="someId" i18n-default-message="This is a label"></span>

Sorry if my comments misled you :)

sebelga · 2019-02-08T12:55:55Z

Ok thanks @maryia-lapata for the explanation. Why not use backtick strings in JS everywhere (apart from multiline)?

Aso, would something like this be ok?

defaultMessage: ['Some very long text',
'that should wrap on multiple lines',
'like this text here'].join(' ')

maryia-lapata · 2019-02-08T13:30:38Z

@sebelga it should be taken into account that all labels are extracted into JSON file to pass it to vendors.i18n validation tool validates and extracts IDs, default messages and descriptions only if they are defined statically and together, otherwise it will fail. The defaultMessage has limitation: it must be string literals, template literals w/o expressions or string-only concatenation expressions, anything else is not allowed. That's why [...].join(' ') won't work.

Since expressions aren't allowed in template literals (

`Hello ${name}`

), I would recommend using backtick strings only for strings with single quotes (for which they were allowed) and not confusing others that such string can be a real template literal (in case someone will need to change label without diving into i18n engine and ICU format).
Anyway it's allowed to use backticks for all labels in defaultMessage.

sebelga · 2019-02-08T13:50:01Z

Perfect, thanks @maryia-lapata for clarifying!

cjcenizal · 2019-02-12T21:13:11Z

Thanks for the reviews @maryia-lapata and @sebelga! Maryia, I've removed the use of double quotes; thanks for catching that.

elasticmachine · 2019-02-12T21:30:42Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-02-13T23:23:49Z

Retest

elasticmachine · 2019-02-14T00:13:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…back

elasticmachine · 2019-02-14T04:05:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…back

elasticmachine · 2019-02-14T05:38:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

maryia-lapata · 2019-02-14T07:49:39Z

@cjcenizal there was changes in i18n tool in #30378, so now build fails when there are unused translations in zh-CN.json file (which was added as well). I guess it's caused by renaming IDs.
The following command should fix unused translations:

$ node scripts/i18n_check.js --fix

cjcenizal · 2019-02-14T16:38:49Z

Thanks @maryia-lapata! I'll try that.

cjcenizal · 2019-02-14T16:49:15Z

@maryia-lapata I removed a few unused Chinese translations. Can you take a look and let me know if this looks right to you?

elasticmachine · 2019-02-14T18:15:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2019-02-14T18:18:04Z

Retest

elasticmachine · 2019-02-14T19:17:52Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Remove unused Chinese translations.

elasticmachine · 2019-06-12T13:21:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal added Project:i18n Feature:CCR and Remote Clusters Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more labels Feb 5, 2019

cjcenizal requested review from jen-huang, sebelga and maryia-lapata February 5, 2019 01:08

cjcenizal changed the base branch from feature/ccr to master February 5, 2019 22:33

cjcenizal added 4 commits February 5, 2019 14:34

WIP

d41c2a6

Apply codemod to convert from intl.formatMessage to i18n.translate.

2667c53

Remove use of injectI18n.

328773e

Address remaining feedback.

8e906a9

cjcenizal force-pushed the ccr/i18n-feedback branch from 16be0ad to 8e906a9 Compare February 5, 2019 22:35

cjcenizal mentioned this pull request Feb 5, 2019

Get rid of injectI18n and rely on global i18n.translate instead #27968

Closed

maryia-lapata approved these changes Feb 6, 2019

View reviewed changes

sebelga approved these changes Feb 8, 2019

View reviewed changes

cjcenizal added v8.0.0 v7.2.0 labels Feb 12, 2019

Remove double-quotes.

e9b7dd5

Merge branch 'master' of github.com:elastic/kibana into ccr/i18n-feed…

88fbdc6

…back

Merge branch 'master' of github.com:elastic/kibana into ccr/i18n-feed…

120fabe

…back

Remove unused Chinese translations.

4d986b7

cjcenizal merged commit 1d2a6fa into elastic:master Feb 15, 2019

cjcenizal deleted the ccr/i18n-feedback branch February 15, 2019 01:37

cjcenizal mentioned this pull request Feb 15, 2019

[7.x] [CCR] i18n feedback (#30028) #31222

Merged

cjcenizal added a commit to cjcenizal/kibana that referenced this pull request Feb 15, 2019

[CCR] i18n feedback (elastic#30028)

4ac3804

* Remove unused Chinese translations.

cjcenizal added a commit that referenced this pull request Feb 15, 2019

[CCR] i18n feedback (#30028) (#31222)

0da5634

* Remove unused Chinese translations.

cjcenizal added the non-issue Indicates to automation that a pull request should not appear in the release notes label May 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] i18n feedback #30028

[CCR] i18n feedback #30028

cjcenizal commented Feb 5, 2019 •

edited

elasticmachine commented Feb 5, 2019

sebelga commented Feb 5, 2019 •

edited

cjcenizal commented Feb 5, 2019 •

edited

elasticmachine commented Feb 5, 2019

maryia-lapata left a comment

maryia-lapata Feb 6, 2019

maryia-lapata Feb 6, 2019

sebelga Feb 8, 2019

maryia-lapata Feb 6, 2019

sebelga Feb 8, 2019

maryia-lapata Feb 6, 2019

maryia-lapata Feb 6, 2019

sebelga left a comment

sebelga Feb 8, 2019

sebelga Feb 8, 2019

sebelga Feb 8, 2019

maryia-lapata commented Feb 8, 2019

sebelga commented Feb 8, 2019

maryia-lapata commented Feb 8, 2019

sebelga commented Feb 8, 2019

cjcenizal commented Feb 12, 2019

elasticmachine commented Feb 12, 2019

cjcenizal commented Feb 13, 2019

elasticmachine commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

maryia-lapata commented Feb 14, 2019

cjcenizal commented Feb 14, 2019

cjcenizal commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

cjcenizal commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

elasticmachine commented Jun 12, 2019

	defaultMessage: "Duplicate leader index pattern aren't allowed."
	defaultMessage: `Duplicate leader index pattern aren't allowed.`

	defaultMessage: "Type and then hit ENTER"
	defaultMessage: 'Type and then hit ENTER'

	defaultMessage: "Index name examples"
	defaultMessage: 'Index name examples'

	defaultMessage: `Unfollow {count} leader indices?`,
	defaultMessage: 'Unfollow {count} leader indices?',

[CCR] i18n feedback #30028

[CCR] i18n feedback #30028

Conversation

cjcenizal commented Feb 5, 2019 • edited

jscodeshift

Codemod

Resources

elasticmachine commented Feb 5, 2019

sebelga commented Feb 5, 2019 • edited

cjcenizal commented Feb 5, 2019 • edited

elasticmachine commented Feb 5, 2019

💔 Build Failed

maryia-lapata 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

sebelga 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

maryia-lapata commented Feb 8, 2019

sebelga commented Feb 8, 2019

maryia-lapata commented Feb 8, 2019

sebelga commented Feb 8, 2019

cjcenizal commented Feb 12, 2019

elasticmachine commented Feb 12, 2019

💔 Build Failed

cjcenizal commented Feb 13, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

elasticmachine commented Feb 14, 2019

💔 Build Failed

elasticmachine commented Feb 14, 2019

💔 Build Failed

maryia-lapata commented Feb 14, 2019

cjcenizal commented Feb 14, 2019

cjcenizal commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💔 Build Failed

cjcenizal commented Feb 14, 2019

elasticmachine commented Feb 14, 2019

💚 Build Succeeded

elasticmachine commented Jun 12, 2019

💔 Build Failed

cjcenizal commented Feb 5, 2019 •

edited

sebelga commented Feb 5, 2019 •

edited

cjcenizal commented Feb 5, 2019 •

edited