Guideline for i18n engine #20620

This doc is really helpful, thanks for putting it together! I put a couple note inline, but some of my comments apply more globally so I figured I would list them here than repeating them over and over inline.

I think we should be consistent with the "message ids" name. In some places we say "message ids", in others "The IDs (names) chosen for message keys", and sometimes "key ID". Since we're using a <FormattedMessage /> component with an id prop, I think "message id" is the most logical choice.
Would you mind replacing all of the “” (fancy quotes) with simple quote marks "?
Nit: Inline code snippets only need two backtick (`) in special cases, would you mind converting the unnecessary double-backticks to singles?
Optional Nit: In several places inline code snippets and quotes are mixed. I think most cases should be either one or the other.

Question: Is it too late to discuss the format for complex messages? What if instead of:

kbn.management.editIndexPattern.scripted.deprecationLangLabel.deprecationLangDetail
kbn.management.editIndexPattern.scripted.deprecationLangLabel.painlessLink

we did something like:

kbn.management.editIndexPattern.scripted.deprecationLangLabel
kbn.management.editIndexPattern.scripted.deprecationLangLabel.painlessLink

This structure is a little less redundant and I think it indicates that the the deprecationLangLabel message is complex, and the painlessLink message goes within it.

spalger · 2018-08-01T16:30:45Z

packages/kbn-i18n/GUIDELINE.md

+This phrase contains a variable, which represents languages list, and a link (``Painless``). For such cases we divide the message into two parts: the main message, which contains placeholders, and additional message, which represents inner message.
+
+It is used the following id naming structure:
+1) the main message id has the type on the second-to-last position, thereby identifying a divided phrase, and the last segment ends with  ``Detail``.


I'm unsure what second-to-last is referring to here.

spalger · 2018-08-01T16:44:28Z

packages/kbn-i18n/GUIDELINE.md

+
+  After translation we get the following structure:
+  ```js
+  <FormattedMessage


I'm super excited by this 😄

spalger · 2018-08-01T16:50:09Z

packages/kbn-i18n/GUIDELINE.md

+
+Each message id should end with a type of the message. For example:
+
+- for header:


I think this section would be more effective if we just listed the message ids and the types. Showing full examples is a little out of scope for what this section is trying to document and makes the actual content a little harder to spot. I think a table presents the information quite nicely:

type example message id

header kbn.management.createIndexPatternHeader

label kbn.management.createIndexPatternLabel

button kbn.management.editIndexPattern.scripted.addFieldButton

drop down kbn.management.editIndexPattern.fields.allTypesDropDown

placeholder kbn.management.createIndexPattern.stepTime.options.patternPlaceholder

aria-label attribute kbn.management.editIndexPattern.removeAria

tooltip kbn.management.editIndexPattern.removeTooltip

error message kbn.management.createIndexPattern.step.invalidCharactersErrorMessage

toggleSwitch kbn.management.createIndexPattern.includeSystemIndicesToggleSwitch

spalger · 2018-08-01T16:52:13Z

packages/kbn-i18n/GUIDELINE.md

+      i18n-values="{ conflictFieldsLength: conflictFields.length }"></span>
+```
+
+In case when `conflictFieldsLength` has value 1, the result string will be `"A field is defined as several types (string, integer, etc) across the indices that match this pattern."`. In case when `conflictFieldsLength` has value 2 or more, the result string - `"2 fields are defined as several types (string, integer, etc) across the indices that match this pattern."`.


In case when conflictFieldsLength has value 1

How about

When `conflictFieldsLength` equals 1

or

In cases where the `conflictFieldsLength` has a value of 1

Same for "In case when conflictFieldsLength has value 2"

spalger · 2018-08-01T17:00:19Z

packages/kbn-i18n/GUIDELINE.md

+
+### Unit tests
+
+Additional adjustments in unit tests are needed only for those components where `injectI18n` component is used. Due to `injectI18n` wraps the target component and passes `intl` via `props`, in tests it is requires to render the target component with `intl` object. That required the rendering the target component using `shallowWithIntl` function from `'test_utils/enzyme_helpers'`.


Additional adjustments in unit tests are needed only for those components where injectI18n component is used. Due to injectI18n wraps the target component and passes intl via props, in tests it is requires to render the target component with intl object. That required the rendering the target component using shallowWithIntl function from 'test_utils/enzyme_helpers'.

I think this reads better as:

When testing React component that use the injectI18n higher-order component, use the shallowWithIntl helper function defined in test_utils/enzyme_helpers to render the component. This will shallow render the component with Enzyme and inject the necessary context and props to use the intl mock defined in test_utils/mocks/intl.

Do you think we should describe what people should do when they want full rendering rather than shallow rendering?

…for js strings, fix description for testing, use uniq "message id" name

yankouskia · 2018-08-02T11:42:57Z

@spalger thank you for such detailed review!
Regarding points:

Absolutely agree, fixed everywhere (message id)
Changed everywhere, where possible, but left code examples in accordance to our code style (quote marks)?
Removed double back tick

Optional Nit: In several places inline code snippets and quotes are mixed. I think most cases should be either one or the other.

Maybe I did not understand this, could you please clarify a little? Or give me an example?
Regarding naming convention, that's a good idea to highlight complex part, but I think it's not a scope of this PR, but we could create issue and discuss it with other devs and translate vendors if it is suitable for them too. What do you think?

elasticmachine · 2018-08-02T12:50:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-08-02T16:48:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

yankouskia · 2018-08-03T09:41:56Z

retest

elasticmachine · 2018-08-03T10:56:00Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

rhoboat · 2018-08-03T14:03:09Z

packages/kbn-i18n/GUIDELINE.md

+
+  ```js
+  <p>
+      The following deprecated languages are in use: {deprecatedLangsInUse.join(', ')}. Support for these languages will be removed in the next major version of Kibana and Elasticsearch. Convert you scripted fields to <EuiLink href={painlessDocLink}>Painless</EuiLink> to avoid any problems.


misspelling: Convert your scripted fields

rhoboat · 2018-08-03T14:03:55Z

packages/kbn-i18n/GUIDELINE.md

+This phrase contains a variable, which represents languages list, and a link (`Painless`). For such cases we divide the message into two parts: the main message, which contains placeholders, and additional message, which represents inner message.
+
+It is used the following message id naming structure:
+1) the main message id has the type on the penultimate position, thereby identifying a divided phrase, and the last segment ends with  `Detail`.


whitespace: extra space before Detail

rhoboat · 2018-08-03T14:04:13Z

packages/kbn-i18n/GUIDELINE.md

+
+```js
+{
+  'kbn.management.editIndexPattern.scripted.deprecationLangLabel.deprecationLangDetail': 'The following deprecated languages are in use: {deprecatedLangsInUse}. Support for these languages will be removed in the next major version of Kibana and Elasticsearch. Convert you scripted fields to {link} to avoid any problems.'


misspelling: your

rhoboat · 2018-08-03T14:06:29Z

packages/kbn-i18n/GUIDELINE.md

+  'kbn.management.editIndexPattern.scripted.table.nameDescription'
+  ```
+
+- For complex messagges, that is divided into several parts, use the folllowing approach:


Grammar: that are divided into several parts
misspelling: messages, following

rhoboat · 2018-08-03T14:07:21Z

packages/kbn-i18n/GUIDELINE.md

+  - the main message id should have the type on the penultimate position, thereby identifying a divided phrase, and the last segment should end with `Detail`,
+  - the inner message id should have the type on the penultimate position and the name of the variable from the placeholder in the main message as the last segment that ends with its own type.
+
+  For example before the translation there was a message:


grammar: For example , before (comma reads better)

rhoboat · 2018-08-03T14:08:55Z

packages/kbn-i18n/GUIDELINE.md

+
+  ```js
+  <strong>Success!</strong>
+  Your index pattern matches <strong>{exactMatchedIndices.length} {exactMatchedIndices.length > 1 ? 'indices' : 'index'}</strong>.


i think this ternary is wrong for english. plural is used for everything but the number 1. so it should be {exactMatchedIndices.length === 1 ? 'index' : 'indices'}

rhoboat · 2018-08-03T14:13:53Z

packages/kbn-i18n/GUIDELINE.md

+
+  `The following dialogue box indicates progress. You can close it and the process will continue to run in the background.`
+
+  If this sentence is separated it’s possible that the context of the `'it'` in `'close it'` will be lost.


I wonder if there's a better word to use here than "sentence" to describe a group. How about paragraph? Or "sentence or group of sentences"? Since the example there is actually two sentences.

rhoboat

Just a couple of nits. Curious what you think, and then I can give another pass through

elasticmachine · 2018-08-06T10:17:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

yankouskia · 2018-08-06T10:22:20Z

@archanid @spalger could you please take a look on updated version?

spalger · 2018-08-07T20:56:52Z

@yankouskia

Optional Nit: In several places inline code snippets and quotes are mixed. I think most cases should be either one or the other.

Maybe I did not understand this, could you please clarify a little? Or give me an example?

I'm referring to things like "`2 indices`" and `"A field is defined as several types (string, integer, etc) across the indices that match this pattern."`. I highlighted it as more optional than the other nits because it's kind of a grey area, but I think both of those cases would be served just fine with double quotes and the inline-code formatting is just a little distracting, especially when the quotes are on the outside of the code snippet.

I think there's a totally legit argument to be made that comments like:

the result string "2 indices"

are identifying a code snippet of a string, as described, so mixing the quotes and code snippet works, but the actual comment is:

the result string - "2 indices"

where the quotes are on the outside of the code snippet and the - is used to create additional visual separation, both of which could be removed if you ask me.

That said, I'm being SUPER nit-picky with this one 😝

rhoboat

Thanks for your work on this!

* Add Guideline for i18n engine * Update Guideline according to changes in i18n engine. * Update examples in Guideline * typo * improve i18n guideline: add table for better view, use single quotes for js strings, fix description for testing, use uniq "message id" name * fix readme description for guideline Co-authored-by: maryia-lapata "mary.lopato@gmail.com"

LeanidShutau · 2018-08-24T09:41:39Z

6.x/6.5: d7b1802

Add Guideline for i18n engine

38d11d0

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

maryia-lapata requested review from epixa, azasypkin, yankouskia and rhoboat July 10, 2018 15:06

Update Guideline according to changes in i18n engine.

1d1a981

epixa removed their request for review July 16, 2018 20:21

maryia-lapata added 2 commits July 20, 2018 16:12

Update examples in Guideline

30bdd62

Merge branch 'master' into i18n-engine-guideline

dcc46de

Merge remote-tracking branch 'kibana/master' into i18n-engine-guideline

c22cda2

typo

12f8b61

yankouskia requested review from spalger and removed request for yankouskia July 31, 2018 10:42

spalger suggested changes Aug 1, 2018

View reviewed changes

yankouskia added 2 commits August 2, 2018 14:28

improve i18n guideline: add table for better view, use single quotes …

7c4d6ff

…for js strings, fix description for testing, use uniq "message id" name

typo

2c89ca4

Merge remote-tracking branch 'kibana/master' into i18n-engine-guideline

ab0cd4d

rhoboat reviewed Aug 3, 2018

View reviewed changes

rhoboat suggested changes Aug 3, 2018

View reviewed changes

fix readme description for guideline

9a5a9d3

azasypkin requested review from spalger and rhoboat August 7, 2018 16:59

spalger approved these changes Aug 7, 2018

View reviewed changes

rhoboat approved these changes Aug 8, 2018

View reviewed changes

yankouskia merged commit 474714a into elastic:master Aug 9, 2018

yankouskia deleted the i18n-engine-guideline branch August 9, 2018 08:38

pavel06081991 mentioned this pull request Aug 17, 2018

[6.x] Guideline for i18n engine (#20620) * Add Guideline for i18n engine * Update Guideline according to changes in i18n engine. * Update examples in Guideline * typo * improve i18n guideline: #22117

Merged

yankouskia added the backported label Aug 20, 2018

LeanidShutau added the v6.5.0 label Aug 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Guideline for i18n engine #20620

Guideline for i18n engine #20620

maryia-lapata commented Jul 10, 2018 •

edited

Loading

elasticmachine commented Jul 10, 2018

elasticmachine commented Jul 16, 2018

elasticmachine commented Jul 20, 2018

yankouskia commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

yankouskia commented Jul 23, 2018

elasticmachine commented Jul 23, 2018

elasticmachine commented Jul 24, 2018

elasticmachine commented Jul 25, 2018

elasticmachine commented Jul 31, 2018

spalger left a comment

spalger Aug 1, 2018

spalger Aug 1, 2018

spalger Aug 1, 2018

spalger Aug 1, 2018

spalger Aug 1, 2018

yankouskia commented Aug 2, 2018

elasticmachine commented Aug 2, 2018

elasticmachine commented Aug 2, 2018

yankouskia commented Aug 3, 2018

elasticmachine commented Aug 3, 2018

rhoboat Aug 3, 2018

rhoboat Aug 3, 2018

rhoboat Aug 3, 2018

rhoboat Aug 3, 2018 •

edited

Loading

rhoboat Aug 3, 2018 •

edited

Loading

rhoboat Aug 3, 2018 •

edited

Loading

rhoboat Aug 3, 2018

rhoboat left a comment

elasticmachine commented Aug 6, 2018

yankouskia commented Aug 6, 2018

spalger commented Aug 7, 2018 •

edited

Loading

rhoboat left a comment

LeanidShutau commented Aug 24, 2018


		Each message id should end with a type of the message. For example:

		- for header:

type	example message id
header	`kbn.management.createIndexPatternHeader`
label	`kbn.management.createIndexPatternLabel`
button	`kbn.management.editIndexPattern.scripted.addFieldButton`
drop down	`kbn.management.editIndexPattern.fields.allTypesDropDown`
placeholder	`kbn.management.createIndexPattern.stepTime.options.patternPlaceholder`
`aria-label` attribute	`kbn.management.editIndexPattern.removeAria`
tooltip	`kbn.management.editIndexPattern.removeTooltip`
error message	`kbn.management.createIndexPattern.step.invalidCharactersErrorMessage`
toggleSwitch	`kbn.management.createIndexPattern.includeSystemIndicesToggleSwitch`


		### Unit tests

		Additional adjustments in unit tests are needed only for those components where `injectI18n` component is used. Due to `injectI18n` wraps the target component and passes `intl` via `props`, in tests it is requires to render the target component with `intl` object. That required the rendering the target component using `shallowWithIntl` function from `'test_utils/enzyme_helpers'`.


		`The following dialogue box indicates progress. You can close it and the process will continue to run in the background.`

		If this sentence is separated it’s possible that the context of the `'it'` in `'close it'` will be lost.

Guideline for i18n engine #20620

Guideline for i18n engine #20620

Conversation

maryia-lapata commented Jul 10, 2018 • edited Loading

elasticmachine commented Jul 10, 2018

💚 Build Succeeded

elasticmachine commented Jul 16, 2018

💚 Build Succeeded

elasticmachine commented Jul 20, 2018

💔 Build Failed

yankouskia commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

💔 Build Failed

yankouskia commented Jul 23, 2018

elasticmachine commented Jul 23, 2018

💔 Build Failed

elasticmachine commented Jul 24, 2018

💔 Build Failed

elasticmachine commented Jul 25, 2018

💚 Build Succeeded

elasticmachine commented Jul 31, 2018

💚 Build Succeeded

spalger 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

yankouskia commented Aug 2, 2018

elasticmachine commented Aug 2, 2018

💔 Build Failed

elasticmachine commented Aug 2, 2018

💔 Build Failed

yankouskia commented Aug 3, 2018

elasticmachine commented Aug 3, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

rhoboat Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

rhoboat Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhoboat left a comment

Choose a reason for hiding this comment

elasticmachine commented Aug 6, 2018

💚 Build Succeeded

yankouskia commented Aug 6, 2018

spalger commented Aug 7, 2018 • edited Loading

rhoboat left a comment

Choose a reason for hiding this comment

LeanidShutau commented Aug 24, 2018

maryia-lapata commented Jul 10, 2018 •

edited

Loading

rhoboat Aug 3, 2018 •

edited

Loading

rhoboat Aug 3, 2018 •

edited

Loading

rhoboat Aug 3, 2018 •

edited

Loading

spalger commented Aug 7, 2018 •

edited

Loading