Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial go at localizing some components #1450

Merged
merged 9 commits into from
Jan 30, 2019

Conversation

chandlerprall
Copy link
Contributor

Converts a number of text locations to use EuiI18n.

Couple notes:

I updated EuiI18n's default prop to accept a function which consumes a passed values object and returns the formatted text/element. The other option (as Rory suggested) is have a formatter function be exposed through the EuiContext. This would let the default messages be forwarded directly to e.g. react-intl. I tried bringing react-intl directly into EUI to allow defaults to be processed by it for a cleaner API, but it doesn't look like you can get JSX in messages without directly using their FormattedMessage component. The other way to go instead is to throw away EuiI18n and use react-intl directly in EUI, with the hard dependency it brings - the API surface is largely the same, basically replacing EuiI18n -> FormattedMessage.

Thoughts @snide @pugnascotia ?

@chandlerprall chandlerprall changed the title Initial go at localizing some components [WIP] Initial go at localizing some components Jan 17, 2019
@snide
Copy link
Contributor

snide commented Jan 17, 2019

@chandlerprall and I chatted briefly over Slack on this. He's gonna reach out to people on the downstream consumer teams to get feedback (since I'm the least helpful here). There's no need to rush into decisions and we can wait a day before going nuts in the repo.

Exciting that even given the syntax questions, we're able to cover so much of the repo so quickly. That's always a good sign :)

This was referenced Jan 17, 2019
@pugnascotia
Copy link
Contributor

So where are we at with react-intl? It's attractive to have less code to maintain, especially when the APIs seem to be converging.

@chandlerprall
Copy link
Contributor Author

EUI will not be including react-intl - there is a necessity to keep our implementation untangled from whatever solution Kibana may be using. I've updated this PR to solidify the described methods of formatting localized strings, and have also added (but need to update the PR description) support for placeholders in those strings, e.g.

<EuiI18n
  token=""
  default="{greeting}, {name}"
  values={{ greeting: 'Hello', name: 'Rory' }}
/>

This PR is technically ready for review but blocked for merging by elastic/kibana#24707 to add enzyme support for React's new context API.

@chandlerprall chandlerprall changed the title [WIP] Initial go at localizing some components Initial go at localizing some components Jan 23, 2019
@chandlerprall
Copy link
Contributor Author

@pugnascotia this is fully ready for review. The conflicts are around the original I18n revert which is now ready to be re-reverted.

@@ -141,11 +142,11 @@ export class EuiCodeEditor extends Component {
filteredCursorStart = cursorStart;
}

const activityLabel = isReadOnly ? 'Interacting' : 'Editing';
const activity = isReadOnly
? 'interacting with the code'
: 'editing';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this getting localised too? Not obvious that it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, activityLabel is used in forming the tokens later. However, how I'm using it here breaks the planned static analysis of tokens, I'll refactor.

When you&rsquo;re done, press Escape to stop {activity}.
<EuiI18n
token={`euiCodeEditor.stop${activityLabel}`}
default={` When you're done, press Escape to stop ${activity}.`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case whitespace matters:

Suggested change
default={` When you're done, press Escape to stop ${activity}.`}
default={`When you're done, press Escape to stop ${activity}.`}

<EuiI18n
token="euiComboBoxOptionsList.createCustomOption"
default="Hit {key} to add {searchValue} as a custom option"
values={{ key: <EuiCode>ENTER</EuiCode>, searchValue: <strong>{searchValue}</strong> }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ENTER be localised?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still localizable, the consumer can ignore that default value in their formatting:

'euiComboBoxOptionsList.createCustomOption': ({ searchValue }) => {
  return i18n.translate(
    'common.ui.euiComboBoxOptionsList.createCustomOption',
    {
      defaultMessage: 'Hit <EuiCode>ENTER</EuiCode> to add {searchValue} as a custom option',
      values: { searchValue // forwarded from EUI's call into this function }
    }
  );
}

Since react-intl allows JSX in the translations, the key value can be ignored and the localized value put directly into the message.

Use the up and down keys to navigate or escape to close.
<EuiI18n
token="euiSuperSelect.screenReaderAnnouncement"
default={`You are in a form selector of {optionsCount} items and must select a single option.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooof, not handling item/items? :-P

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't going to refactor any logic as part of this :-D

values?: I18nTokenShape<T>['values']
): ReactChild {
const renderable = (i18nMapping && i18nMapping[token]) || valueDefault;
if (typeof renderable === 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal style choice - since each branch here returns, I'd remove the else nits. No big deal though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to remove else blocks.

appendValueToChildren(child);
child = {propName: ''};
} else if (char === '}') {
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated code to use a type cast instead of completely ignoring the line to better communicate intent.

@chandlerprall chandlerprall merged commit b047de8 into elastic:master Jan 30, 2019
@chandlerprall chandlerprall deleted the feature/i18n-tokens branch January 30, 2019 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants