Localization "Index Patterns" tab #20525

maryia-lapata · 2018-07-06T16:48:02Z

It's integration "Index Patterns" tab and I18n engine
#19729

yankouskia · 2018-07-09T07:08:36Z

packages/__mocks__/@kbn/i18n.js

+const I18nBrowser = require.requireActual('../../kbn-i18n/src/browser');
+const I18nNode = require.requireActual('../../kbn-i18n/src/index');
+
+module.exports = {


Note: workaround under current implementation (resolve client libraries for node env)

yankouskia · 2018-07-09T07:11:39Z

src/ui/public/management/section_translate_helper.js

+import { sectionTypes } from './section_keys';
+
+export function translateSectionName(i18n, typeId) {
+  if (!typeId) {


I'd remove it and return empty string in default case

yankouskia · 2018-07-09T07:15:57Z

src/test_utils/public/enzyme_helpers.js

+
+const intl = {
+  formatMessage: jest.fn().mockImplementation(({ defaultMessage }) => defaultMessage),
+  formatDate: jest.fn().mockImplementation(({ defaultMessage }) => defaultMessage),


let's change default implementation if we really need these functions (formatDate for instance don't have defaultMessage)

Or remove unnecessary properties at all and just change for tests contextType for Intl

yankouskia · 2018-07-09T07:16:30Z

src/dev/precommit_hook/casing_check_config.js

@@ -53,7 +53,7 @@ export const IGNORE_FILE_GLOBS = [
 * @type {Array}
 */
 export const KEBAB_CASE_DIRECTORY_GLOBS = [
-  'packages/*',
+  'packages/!(__mocks__)',


According to Jest, if the module you are mocking is a Node module, the mock should be placed in the __mocks__ directory. At the same time there is a rule in kibana in which files under packages should use kebab case.
Since mocking the @kbn/i18n module is a workaround, after merging the PR #20513 I will revert this change.

yankouskia · 2018-07-09T07:18:30Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

-
-        {this.renderDeleteConfirmationModal()}
-      </div>
+      <I18nProvider>


it's not root component, you don't need provider here. let's use Provider only in ReactDOM.render calls

yankouskia · 2018-07-09T07:22:54Z

src/test_utils/public/enzyme_helpers.js

+  now: jest.fn().mockImplementation(({ defaultMessage }) => defaultMessage),
+};
+
+export function shallowIntl(node, { context, childContextTypes, ...props } = {}) {


maybe let's rename to shallowWithIntl for more consistency?

…heck for translation type id

…a/kibana into i18n-index-patterns

bmcconaghy

This is coming along very nicely, just have a few concerns I've documented here, the primary one being creating a new object inside values each time (I only commented on this in one spot, but it happens a lot in this code).

bmcconaghy · 2018-07-24T13:22:06Z

...anagement/sections/indices/create_index_pattern_wizard/components/empty_state/empty_state.js

+              id="kbn.management.createIndexPattern.emptyStateLabel.emptyStateDetail"
+              defaultMessage="{needToIndex} {learnHowLink} or {getStartedLink}"
+              values={{
+                needToIndex: (


I think I would prefer the values object here to be declared as a constant above and then referenced here. This would make things easier to read, and would also prevent a new object being created with each rerender. See this for an explanation of why that might be bad: https://medium.com/@esamatti/react-js-pure-render-performance-anti-pattern-fb88c101332f

@bmcconaghy Basically, this is common pattern for avoiding creation of new object, but I would not say, that in this case, because:

Moving such objects to constants will decrease readability of code in general

Some of values are React components too and use props. So if we move values to constant outside component we will not get props, if we will create it inside render it will be the same one

This case is envisaged by react-intl guys. And for avoiding rerendering they espacially use shallow compare for values props. (Here)[https://github.com/yahoo/react-intl/blob/master/src/components/message.js#L43]

Fair enough on the performance point. I actually find the FormattedMessage nested in FormattedMessage harder to read personally, as I get lost in the multiple levels of nesting, so would still like to see the values defined outside of the FormattedMessage and then referenced there.

It feels like the right solution for this readability problem would be integrating i18n into EUI components at some point, so that this:

<EuiTextColor color="subdued"> <FormattedMessage id="kbn.management.createIndexPattern.emptyStateLabel.needToIndexLabel" defaultMessage="You'll need to index ....." /> </EuiTextColor>

can be changed to:

<EuiTextColor color="subdued" i18n-id="kbn.management.createIndexPattern.emptyStateLabel.needToIndexLabel"> You'll need to index ..... </EuiTextColor>

We'll still have to use values to keep all parts of the single message together though (can be quite important for RTL languges).

But that's a bigger effort that spans several teams and i18n should prove itself in practice first.

bmcconaghy · 2018-07-24T13:32:34Z

...dex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.js

+              <strong>
+                <FormattedMessage
+                  id="kbn.management.createIndexPattern.step.status.matchAnyLabel.strongIndicesLabel"
+                  defaultMessage="{allIndicesLength, plural, one {# index} other {# indices}}"


I think we could clean up the message here for the case when a single index exists. "any of your 1 index" sounds pretty silly. "any of your" should be made part of the conditional. I would suggest the messages should be: singular "...can match your index below" plural (remains the same)

bmcconaghy · 2018-07-24T13:33:51Z

...dex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.js

-        You can match any of your <strong>{allIndices.length} indices</strong>, below.
+        <FormattedMessage
+          id="kbn.management.createIndexPattern.step.status.notMatchLabel.notMatchDetail"
+          defaultMessage="The index pattern you've entered doesn't match any indices. You can match any of your {strongIndices}, below."


same issue as above

bmcconaghy · 2018-07-24T13:42:24Z

...na/public/management/sections/indices/create_index_pattern_wizard/lib/extract_time_fields.js

@@ -31,8 +36,11 @@ export function extractTimeFields(fields) {
    display: '───',
    fieldName: '',
  };
+  const noTimeFieldLabel = i18n.translate('kbn.management.createIndexPattern.stepTime.noTimeFieldOptionLabel', {
+    defaultMessage: 'I don\'t want to use the Time Filter'


I would prefer that we use `` or "" for string delimiting if the string contains a ' character. Easier to read than using escaping.

@bmcconaghy Actually, I agree with you that it sometimes maybe better for readabilty (and we use it in react components), but there are points why we did it in such way.

We have eslint rule: "Strings must use singlequote"

Template string cannot be used, because this restriction is set by tool for extracting messages and verifying them

I guess I can live with this as it is. Is there a possibility of modifying the tool so that template strings would work?

Basically there is possibility to do it, but babel creates quite hard AST with template strings, which is difficult to work with, so currently we have decided to use such approach

Yeah, we just decided not to complicate things for now. If we allow using template strings for i18n ID's and default messages, someone will try to use it as a template string with dynamic parameter some day and then we'll have troubles :/

bmcconaghy · 2018-07-24T13:43:36Z

...public/management/sections/indices/edit_index_pattern/create_edit_field/create_edit_field.js

-          toastNotifications.add(`'${this.indexPattern.title}' index pattern doesn't have a scripted field called '${fieldName}'`);
+          const message = i18n.translate('kbn.management.editIndexPattern.scripted.noFieldLabel',
+            {
+              defaultMessage: '\'{indexPatternTitle}\' index pattern doesn\'t have a scripted field called \'{fieldName}\'',


Again prefer other quotation marks here over escaping.

elasticmachine · 2018-07-24T17:54:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang

LGTM, I spot checked the changed files and ran branch locally and confirmed there were no functional regressions.

azasypkin

Looks good to me, just a couple of issues and a few nits/questions.

...dex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.js

+        <FormattedMessage
+          id="kbn.management.createIndexPattern.step.status.notMatchLabel.notMatchDetail"
+          defaultMessage="The index pattern you've entered doesn't match any indices.
+          You can match {indicesLength, plural, one {your} other {any of your}} {strongIndices}, below."


azasypkin · 2018-07-26T12:35:35Z

...wizard/components/step_index_pattern/__tests__/__snapshots__/step_index_pattern.test.js.snap

  characterList="\\\\, /, ?, \\", <, >, |"
  data-test-subj="createIndexPatternStep1Header"
  errors={
    Array [
-      "An index pattern cannot contain spaces or the characters: \\\\, /, ?, \\", <, >, |",
+      "An index pattern cannot contain spaces or the characters: {characterList}",


issue: having exact chars in the snapshot like it was before is important, can you please tweak test so that it properly expands {characterList}?

azasypkin · 2018-07-26T12:51:46Z

...dex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.js

-          Your index pattern can match any of your <strong>{allIndices.length} indices</strong>, below.
+          <FormattedMessage
+            id="kbn.management.createIndexPattern.step.status.matchAnyLabel.matchAnyDetail"
+            defaultMessage="Your index pattern can match any of your {strongIndices}, below."


note: there is no need to change anything in this PR, but for the following PRs: parameter name and message id should rather reflect what this parameters/message means, not how it looks like visually (strongIndicies vs indexCount). It will help localizers to understand context better as well.

...dex_pattern_wizard/components/step_index_pattern/components/status_message/status_message.js

-          Your index pattern can match any of your <strong>{allIndices.length} indices</strong>, below.
+          <FormattedMessage
+            id="kbn.management.createIndexPattern.step.status.matchAnyLabel.matchAnyDetail"
+            defaultMessage="Your index pattern can match any of your {strongIndices}, below."


...s/create_index_pattern_wizard/components/step_time_field/components/time_field/time_field.js

 }) => (
  <EuiForm>
    { isVisible ?
      <EuiFormRow
        label={
          <EuiFlexGroup gutterSize="xs" justifyContent="spaceBetween" alignItems="center">
            <EuiFlexItem grow={false}>
-              <span>Time Filter field name</span>


azasypkin · 2018-07-26T15:07:19Z

src/ui/public/field_editor/components/scripting_help/scripting_syntax.js

+                target="_window"
+                href={getDocLink('scriptedFields.painless')}
+              >
+                <FormattedMessage id="common.ui.fieldEditor.syntax.defaultLabel.painlessLink" defaultMessage="Painless" />&nbsp;


note: same question/note about Painless here.

azasypkin · 2018-07-26T15:10:48Z

src/ui/public/field_editor/field_editor.js

+                language: <EuiCode>{field.lang}</EuiCode>,
+                painlessLink: (
+                  <EuiLink target="_window" href={getDocLink('scriptedFields.painless')}>
+                    <FormattedMessage


note: same

azasypkin · 2018-07-26T15:13:17Z

src/ui/public/field_editor/field_editor.js

@@ -296,16 +335,29 @@ export class FieldEditor extends PureComponent {
    const { field, fieldTypeFormats, fieldFormatId, fieldFormatParams } = this.state;
    const { fieldFormatEditors } = this.props.helpers;
    const defaultFormat = fieldTypeFormats[0] && fieldTypeFormats[0].resolvedTitle;
+    const label = defaultFormat


question: likely I missed some related review comment from other folks, so just double checking: previously we had null if defaultFormat was not defined, now we render Format label: is that expected?

Actually this is the same. Maybe you missed <span> outside?

Ugh, my bad, sorry :/

azasypkin · 2018-07-26T15:24:10Z

src/ui/public/index_patterns/_field.js

    toastNotifications.addDanger({
-      title: `Unknown field type ${spec.type}`,
-      text: `Field ${spec.name} in indexPattern ${indexPattern.title} is using an unknown field type.`
+      title: title,


nit: use shortcuts maybe?

title, text

azasypkin · 2018-07-26T15:27:27Z

src/ui/public/index_patterns/_index_pattern.js

+        });
+
+        const warningText = i18n.translate('common.ui.indexPattern.warningText', {
+          defaultMessage: 'For more information, view the ["{title}" index pattern in management]({link})',


question: was there any agreement to change the way we display that link?

elasticmachine · 2018-07-27T14:23:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin

LGTM, but please wait for green CI.

elasticmachine · 2018-07-30T13:20:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

yankouskia · 2018-07-30T13:39:53Z

retest

elasticmachine · 2018-07-30T14:59:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Integrate i18n-engine into "Index Patterns" tab * Adjusted unit tests for "Index Patterns" tab * Localization of "Index patterns" section name * Rename the function shallowIntl to shallowWithIntl, remove needless check for translation type id * Refactoring of default message * Localization for FieldEditor component * Adjust unit tests for FieldEditor component * Integrate i18n-engine into "Index Patterns" tab * Adjusted unit tests for "Index Patterns" tab * Localization of "Index patterns" section name * Rename the function shallowIntl to shallowWithIntl, remove needless check for translation type id * Refactoring of default message * Localization for FieldEditor component * Adjust unit tests for FieldEditor component * Replace I18nContext to injectI18n according to changes in @kbn/i18n * Fix broken saving form * Adjust components importing according to changes in @kbn/i18n * Formatting and refactoring * Update ids * Fix invalid HTML and refactoring * Use i18n module instead of AngularJS service * Localize scripting_syntax.js, refactoring * Update message ids. * fix plural form in status messages * fix messages in status message, _field component * move back span in time_field * refactor enzyme helper for providing intl into context * do not translate Painless * test call params in formatMessage * clear formatMessage mock after each test Co-authored-by: maryia-lapata <[email protected]>

LeanidShutau · 2018-08-06T12:15:13Z

6.x/6.5: f6f4e68

maryia-lapata added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Project:i18n labels Jul 6, 2018

maryia-lapata requested review from epixa, azasypkin, yankouskia and rhoboat July 6, 2018 16:48

yankouskia reviewed Jul 9, 2018

View reviewed changes

maryia-lapata added review and removed review labels Jul 9, 2018

maryia-lapata added 5 commits July 10, 2018 18:15

Integrate i18n-engine into "Index Patterns" tab

f99eca7

Adjusted unit tests for "Index Patterns" tab

d42e69d

Localization of "Index patterns" section name

63c798a

Rename the function shallowIntl to shallowWithIntl, remove needless c…

2904a6f

…heck for translation type id

Refactoring of default message

463490f

maryia-lapata force-pushed the i18n-index-patterns branch from 2e53185 to 463490f Compare July 10, 2018 15:16

maryia-lapata added the review label Jul 11, 2018

Localization for FieldEditor component

e29cf5f

maryia-lapata added review and removed review labels Jul 12, 2018

Adjust unit tests for FieldEditor component

f186b43

maryia-lapata force-pushed the i18n-index-patterns branch from 50d5ac9 to f186b43 Compare July 13, 2018 06:52

maryia-lapata removed the review label Jul 13, 2018

maryia-lapata added 8 commits July 13, 2018 13:04

Integrate i18n-engine into "Index Patterns" tab

4514ee8

Adjusted unit tests for "Index Patterns" tab

d41b9cb

Localization of "Index patterns" section name

512ba8a

Rename the function shallowIntl to shallowWithIntl, remove needless c…

c32cfe0

…heck for translation type id

Refactoring of default message

62d35af

Localization for FieldEditor component

407cdda

Adjust unit tests for FieldEditor component

6ad10c1

Merge branch 'i18n-index-patterns' of https://github.com/maryia-lapat…

d92d137

…a/kibana into i18n-index-patterns

bmcconaghy suggested changes Jul 24, 2018

View reviewed changes

fix plural form in status messages

4cd6f0c

bmcconaghy approved these changes Jul 24, 2018

View reviewed changes

jen-huang approved these changes Jul 25, 2018

View reviewed changes

azasypkin requested changes Jul 26, 2018

View reviewed changes

yankouskia added 4 commits July 27, 2018 13:46

fix messages in status message, _field component

031aa64

move back span in time_field

1f4711a

refactor enzyme helper for providing intl into context

87fd943

do not translate Painless

bf4a7f8

yankouskia added 2 commits July 30, 2018 14:53

test call params in formatMessage

82a2e9d

clear formatMessage mock after each test

04359eb

azasypkin approved these changes Jul 30, 2018

View reviewed changes

yankouskia merged commit 7063dbc into elastic:master Jul 30, 2018

yankouskia deleted the i18n-index-patterns branch July 30, 2018 18:33

LeanidShutau added the backport pending label Aug 3, 2018

LeanidShutau mentioned this pull request Aug 3, 2018

[6.x] Localization "Index Patterns" tab (#20525) #21637

Merged

LeanidShutau added backported v6.5.0 and removed backport pending labels Aug 6, 2018

jen-huang mentioned this pull request Aug 16, 2018

Fix color and static lookup field formatters not loading #22044

Merged

maryia-lapata mentioned this pull request Aug 24, 2018

[I18n] Index Pattern localization #19729

Closed

Localization "Index Patterns" tab #20525

Localization "Index Patterns" tab #20525

Conversation

maryia-lapata commented Jul 6, 2018 • 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

Choose a reason for hiding this comment

bmcconaghy 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

azasypkin Jul 26, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 24, 2018

💚 Build Succeeded

jen-huang left a comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

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

elasticmachine commented Jul 27, 2018

💚 Build Succeeded

azasypkin left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 30, 2018

💔 Build Failed

yankouskia commented Jul 30, 2018

elasticmachine commented Jul 30, 2018

💚 Build Succeeded

LeanidShutau commented Aug 6, 2018

maryia-lapata commented Jul 6, 2018 •

edited

Loading

azasypkin Jul 26, 2018 •

edited

Loading