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

EuiContext & I18n #1404

Merged
merged 17 commits into from
Jan 17, 2019
Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Jan 4, 2019

Summary

Opening this for API review.

from #738

Adds internationalization support via React context.

language toggle example

src-docs/src/views/context/context.js contains the context & i18n usage examples:

EuiContext

EuiContext is used to provide a token->translation mapping and a number formatter for EUI components to use.

I18n

I18n component uses the provided token mapping or falls back to the provided default (english) value. I went with I18n over EuiI18n for readability and to partially infer it shouldn't be used outside of EUI. Not tied to this opinion and could be easily persuaded otherwise.

I18n supports three use cases:

  1. simple text injection
<I18n token="token.name" default="english text for this value"/>
  1. single token lookup for use in a value
<I18n token="token.name" default="english text for this value">
  {tokenValue => <EuiFieldText placeholder={tokenValue}/>}
</I18n>
  1. multi-token lookup, available when token density is high and would otherwise cause cluttered code
<I18n tokens={['token1', 'token2']} defaults={['default1', 'default2']}>
  {([value1, value2]) => (
    <div>
      {value1}<br/>
      <EuiFieldText placeholder={value2}/>
    </div>
  )
</I18n>

supported value types

The tokens can be mapped to any valid ReactChild - i.e. string, number, or JSX - both in the defaults or in the provided mapping.

<I18n token="token.name" default={<div className="special">Hello World</div>}/>

proposal for token names

I see two options:

  1. [component name].[token description] EuiComboBox.placeholder
  2. [component type].[token description] combobox.placeholder

I prefer the second way as it creates less turbulence in token names if a token usage moves, e.g. moving the placeholder lookup from EuiComboBox into EuiComboBoxInput

I18nNumber

I18nNumber component uses the provided formatNumber function and supports the same three use cases as I18n.

  1. single value formatting & injection
<I18nNumber value={1500}/>
  1. single value formatting as a render prop
<I18n value={1500}>
  {formattedValue => <EuiFieldText placeholder={formattedValue}/>}
</I18n>
  1. multi-value formatting
<I18n values={[1500, 2000]}>
  {([value1, value2]) => (
    <div>
      {value1} : {value2}
    </div>
  )
</I18n>

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@snide
Copy link
Contributor

snide commented Jan 4, 2019

Know this is early but this method looks good to me. It's still pretty readible. Most complicated text I can think of EUI is in the EuiSuperDatePicker. We might want to just check through to make sure it would work someplace like there. Otherwise comments are more on how we should set up docs for this.

@pugnascotia
Copy link
Contributor

This looks good from an initial look over. I'm wondering about translations that contain values, for example EuiFilePicker. What does an <I18n> look like if it needs to print a value e.g. a number or date?

@chandlerprall
Copy link
Contributor Author

chandlerprall commented Jan 7, 2019

@pugnascotia good thinking

How about instead of passing the explicit mappings (<Context mappings={translations[language]}>) you pass all the mappings and a locale:

<Context mappings={transpations} locale={language}>

This would allow underlying components to rely on the browser built-in Intl api for formatting numbers & dates based on the locale. This would add a requirement that the keys on the mappings object are a BCP 47 language tag - unless the mapping object is passed in addition to the locale.

The alternative would be to have those translation functions be passed into EUI's context - <Context mappings={translations[language]} formatDate={dateFn} formatNumber={numberFn}//>

@thompsongl
Copy link
Contributor

Using standard language tags and formalizing the locale as a prop makes sense here, I think.
It still leaves the question of how best to interface with the Intl API and whether to expose custom formatter overrides, but those details can both change if the translation object + locale tag situation is easy to deal with.

@chandlerprall
Copy link
Contributor Author

@pugnascotia @thompsongl I've updated the PR & description and added an I18nNumber component to provide decimal formatting. I allowed the formatting to be customized by a custom function passed to EuiContext as I believe it provides the greater flexibility to downstream projects & their UX & users -

  render() {
    const i18n = {
      mapping: mappings[this.state.language],
      formatNumber: (value) => new Intl.NumberFormat(this.state.language).format(value),
    };

    return (
      <EuiContext i18n={i18n}>...</EuiContext>
    );
  }

@cchaos
Copy link
Contributor

cchaos commented Jan 8, 2019

Designer perspective

...so do with what you will

"Context"

Without knowing that behind-the-scenes it uses React context, I wouldn't think of EuiContext being used for localization. But maybe this is more clear to React aficionados.

"I18n"

I'd still prefix with Eui so that it's not confused with or misunderstood to be from some other library or dependency.

Docs

You did so well explaining it in the PR summary, can you add more to the actual documentation page?

Component example

Can you update one component that had hard-coded text to use this so we can see an example of how to write them moving forward?

@chandlerprall
Copy link
Contributor Author

Thanks!

"Context"
Without knowing that behind-the-scenes it uses React context, I wouldn't think of EuiContext being > used for localization. But maybe this is more clear to React aficionados.

You'll probably never need to know about EuiContext unless you're wrapping EUI in another React app and want to support I18n in (Kibana & Cloud will do this at a top level).

"I18n"
I'd still prefix with Eui so that it's not confused with or misunderstood to be from some other library or dependency.

👍

Docs
You did so well explaining it in the PR summary, can you add more to the actual documentation page?

Definitely - this was only meant to showcase the API, a full docs page for the i18n components is coming!

Component example
Can you update one component that had hard-coded text to use this so we can see an example of how to write them moving forward?

👍

@chandlerprall
Copy link
Contributor Author

I've added the Eui prefix to the component names and updated EuiTablePagination to show how this would be used in an existing component https://github.com/elastic/eui/pull/1404/files#diff-d71c0430e837b165e398085f4cd16d5c

@@ -49,7 +50,7 @@ export class EuiTablePagination extends Component {
iconSide="right"
onClick={this.onButtonClick}
>
{`Rows per page: ${itemsPerPage}`}
<EuiI18n token="pagination.rowsPerPage" default="Rows per page"/>: {itemsPerPage}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does this pagination token reside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, it does not (nor would EUI define it elsewhere). It would be the key for a translation passed in from e.g. Kibana. The I18n component takes the token and looks it up in the translation map, if it doesn't exist then the default value is used. There will be tooling built to ensure validity of tokens and to track changes (Cloud has a similar setup).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so this is where I get confused. So then consumers need to create exactly a

const pagination = {
  rowsPerPage = "Rows per page"
}

?

And then how do the pass it to the component.

Ack, I'm sorry these are probably some very basic questions, but I've never dealt with localization before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally valid questions! This is where EuiContext comes in - context in React is a value that is invisibly passed down to components; those components can either ignore it or opt into receiving the value in addition to their props. This allows translations (context) to be set at a top level and then EUI components simply subscribe to it through EuiI18n. If that context is missing, it will fallback to the default prop.

This means Kibana, Cloud, etc would need to collate the EUI-related translations into a map of {token: copy} and pass it to EuiContext at a high-level in their application (generally these things wrap even the top-most DOM node in a React app). This is the mechanism used by react-router to inform components what page they're on, or redux to invisibly pass state down to subscribing components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a zoom walkthrough next week?

Copy link
Contributor

Choose a reason for hiding this comment

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

But I"m not a blocker, just trying to understand

@pugnascotia
Copy link
Contributor

I sort-of feel like if you're going to allow formatNumber etc to be specified on the context, them formatMessage should be allowed. I expect (but have not verified) that we'd need that to wire this into react-intl.

@chandlerprall
Copy link
Contributor Author

chandlerprall commented Jan 10, 2019

I've collated the occurrences of hard-coded text inside JSX, and potential text locations in JSX expressions & attributes (not an exhaustive list of all possible locations, but the normal places to look) - https://gist.github.com/chandlerprall/5e0f9ceca7e89abc11e1b1d99ac89fca

@pugnascotia I'm not sure how much benefit that provides, there are very, very few places in EUI where that kind of interpolation of values would be beneficial. Looking through the places we have hard coded values, there's less than 5 places I see where we even co-locate values, and even then it isn't mixed together like text {value} text, the value is always left or right of the copy. Not saying providing a formatMessage function is a bad idea, but not sure how valuable it is to EUI - if you have some examples of where you think it'd be good I'd love to see them!

@chandlerprall
Copy link
Contributor Author

jenkins test this

@pugnascotia
Copy link
Contributor

@chandlerprall let's just proceed with your design, we'll soon discover if it needs refining when we wire it into Cloud.

@chandlerprall chandlerprall changed the title [WIP] EuiContext & I18n EuiContext & I18n Jan 16, 2019
@chandlerprall
Copy link
Contributor Author

@snide @cchaos @ryankeairns @pugnascotia this is ready for a final review

Next steps are to find hard coded text and replace with EuiI18n as well as tooling & documentation around tokens.

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

We did a walkthru with the designers on this yesterday and I think it works. We can adjust as we move forward into the tokenization, but I think the core concepts work for what we need.

@chandlerprall chandlerprall merged commit 57d4433 into elastic:master Jan 17, 2019
@chandlerprall chandlerprall deleted the feature/738-i18n branch January 17, 2019 18:06
snide added a commit that referenced this pull request Jan 17, 2019
snide added a commit to snide/eui that referenced this pull request Jan 17, 2019
snide added a commit that referenced this pull request Jan 17, 2019
* Revert "EuiContext & I18n (#1404)"

This reverts commit 57d4433.
@snide snide mentioned this pull request Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants