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

Some controls needs to support i18n #738

Closed
pugnascotia opened this issue Apr 30, 2018 · 18 comments
Closed

Some controls needs to support i18n #738

pugnascotia opened this issue Apr 30, 2018 · 18 comments

Comments

@pugnascotia
Copy link
Contributor

Since EUI is now providing controls that contain text (i.e. combo box), we need to think about how to allow for internationalisation. Cloud UI is already using react-intl heavily, but I don't think EUI should have any opinion over how the i18n is done.

I wondered if it might work to have an extra prop, e.g. translations, which is an object of all the required strings (or more precisely React nodes, to allow for custom formatting / content). The prop types would document the required keys and the default values.

cc @cjcenizal

@cjcenizal
Copy link
Contributor

@pugnascotia Could you give an example of what this might look like from a consumption standpoint? I'd like to see what the interface looks like from the outside, and what these props would look like.

@snide snide mentioned this issue Apr 30, 2018
44 tasks
@pugnascotia
Copy link
Contributor Author

So I'm thinking that EUI would push all responsibility for generating the correct text to the app, so no locale-switch, any of that because it's tied into how your app handles i18n. That just leaves picking out the appropriate translations, and providing any values that are needed for formatting in a callback.

Basic example (probably what EUI would use as a default):

const translations = {
  loadingOptions: `Loading options`,
  alreadyAdded: value => <span><strong>{value}</strong> has already been added</span>,
  addOption: value => <span>Hit <EuiCode>ENTER</EuiCode> to add <strong>{value}</strong> as a custom option</span>,
  noOptions: `There aren&rsquo;t any options available`,
  allOptionsSelected: `You&rsquo;ve selected all available options`,
}

// Then:

<EuiComboBox
  { ...otherProps }
  translations={ translations } />

Cloud UI example using react-intl:

const translations = {
  loadingOptions: <FormattedMessage
    id='foo.loadingOptions'
    defaultMessage='Loading options' />,

  alreadyAdded: value => <FormattedMessage
    id='foo.alreadyAdded'
    defaultMessage='{option} has already been added'
    values={{
      option: <strong>{ value }</value>,
    }} />,

  addOption: value => <FormattedMessage
    id='foo.addOption'
    defaultMessage='Hit {key} to add {option} as a custom option'
    values={{
      key: <EuiCode>ENTER</EuiCode>, // Should intl this too, but this is a demo
      option: <strong>{ value }</value>,
    }} />,

  noOptions: <FormattedMessage
    id='foo.noOptions'
    defaultMessage='There aren&rsquo;t any options available' />,

  allOptionsSelected: <FormattedMessage
    id='foo.allOptionsSelected'
    defaultMessage='You&rsquo;ve selected all available options' />,
}

@cjcenizal
Copy link
Contributor

Nice! I think this is the right pattern. Though I think we should change the prop name to something i18n-agnostic, like "messages" or "microcopy", since the prop is essentially about customizing the text in the component (uncoupled to internationalization).

@pugnascotia
Copy link
Contributor Author

Ah yeah, good point. There's no reason at all to call it translations :-D

@pugnascotia
Copy link
Contributor Author

I noticed that EuiToast now has a screen-ready-only message in English. This implies that any a11y messages that EUI embeds must be localisable.

@cjcenizal
Copy link
Contributor

Good point Rory. It seems like messages is a broad enough prop name to encompass both microcopy and screen-reader-only text.

@pugnascotia
Copy link
Contributor Author

I'm actually wondering now if, in time, we might need an <EuiProvider> or something like that where you can configure a context for EUI that holds this stuff, like react-intl uses. I'm assuming we wouldn't want to have to specify the toast screen-reader message every single time we created a toast, for example.

I'm just floating ideas, BTW, I haven't thought it through.

@cjcenizal
Copy link
Contributor

Kibana wraps its toasts in an abstraction, which I think would be the right place to configure these messages. Encapsulated there, consumers would never need to know about them. We should probably do an audit of all of the places these messages would need to be defined before spitballing solutions, but it's possible that similar abstractions could serve the same purpose everywhere these messages are required.

@chandlerprall
Copy link
Contributor

@cjcenizal can you link an example of how Kibana does it?

@cjcenizal
Copy link
Contributor

cjcenizal commented May 22, 2018

It's a little convoluted:

If anyone is interested, here's where the interface is documented: https://github.com/elastic/kibana/blob/master/src/ui/public/notify/toasts/TOAST_NOTIFICATIONS.md

@nreese nreese mentioned this issue Dec 17, 2018
10 tasks
@chandlerprall
Copy link
Contributor

chandlerprall commented Dec 19, 2018

Talked with @azasypkin about i18n+EUI, our plan is to create an EuiContext component that has an interface for specifying a map of tokens -> ReactNode (text, JSX, etc), with EUI's current english strings moved into a default map. EUI components will lookup against the map when rendering. This would require creating a component in Kibana (and elsewhere) that wraps EuiContext to provide the map, and including that wrapping component everywhere a React app is instantiated.

@snide targeting 6.7, there's a translation effort happening around mid-January so we'll want our strings identified by then and initial implementation available for testing

@timroes how much effort (one time vs. on-going) would it take to maintain & use a wrapping component in Kibana, considering our angular/react codebase.

@pugnascotia same question as above, would this work for Cloud?

@cjcenizal
Copy link
Contributor

@chandlerprall That sounds great. All of the newer Elasticsearch apps are written as standalone React apps with clear mounting points (e.g. Index Management), so your solution sounds like it would be pretty easy to implement for us.

@pugnascotia
Copy link
Contributor Author

That should work from a Cloud POV. We already have a top-level point where we add context providers, so adding <EuiContext messages={ euiMessages }> or whatever there would be easy.

@snide snide mentioned this issue Dec 26, 2018
19 tasks
@azasypkin
Copy link
Member

/cc @ppisljar as well, to make sure #738 (comment) works for the Kibana Apps team.

@ppisljar
Copy link
Member

ppisljar commented Jan 3, 2019

our apps are not in react yet, but i guess the only difference for us is that we would need to be a bit more careful to actually pass EuiContext wrapper to every place we use react ?

@azasypkin
Copy link
Member

but i guess the only difference for us is that we would need to be a bit more careful to actually pass EuiContext wrapper to every place we use react ?

Yeah, only on every React -> DOM or React -> Angular "boundary". Technically we do this already with I18nProvider, so we can just combine it with EuiContext into another top level component that we'll use instead of I18nProvider.

@chandlerprall chandlerprall mentioned this issue Jan 4, 2019
10 tasks
@chandlerprall
Copy link
Contributor

API and Usage proposal is up at #1404 for review.

@snide
Copy link
Contributor

snide commented Feb 16, 2019

Closing this. We have the pieces in place for this. It's just the tokenization at this point, which would be better served by another issue.

@snide snide closed this as completed Feb 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants