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

Allow EuiI18n | string everywhere translated text is expected #2474

Closed
pickypg opened this issue Oct 22, 2019 · 4 comments · Fixed by #3749
Closed

Allow EuiI18n | string everywhere translated text is expected #2474

pickypg opened this issue Oct 22, 2019 · 4 comments · Fixed by #3749

Comments

@pickypg
Copy link
Member

pickypg commented Oct 22, 2019

While reviewing a PR making use of EuiI18n, I came across this fairly boilerplate code making use of the render method to get the translated text:

 <EuiI18n
  token="euiContext.openMenu"
  default="Open menu"
>
  {(openMenu: string) => (
    <EuiHeaderLogo
      iconType="apps"
      aria-label={openMenu}
      onClick={() => toggleOpen()}
    />
  )}
</EuiI18n>

It got me wondering, can we not generically allow HTML attributes to be specified as EuiI18n | string parameters, thus bridging the gap and enabling simpler usage of translation?

<EuiHeaderLogo
  iconType="apps"
  aria-label={
    <EuiI18n
      token="euiContext.openMenu"
      default="Open menu"
    />
  }
  onClick={() => toggleOpen()}
/>

Then could we take this a step further and push that into all EUI components, like EuiNavDrawerGroup's labels.

@cchaos
Copy link
Contributor

cchaos commented Mar 18, 2020

I have, as of late, been frustrated that we don't also just have a simple function that returns a string for this. Kibana does have something like this:

title={i18n.translate('xpack.lens.indexPattern.noPatternsLabel', {
  defaultMessage: 'No index patterns',
})}

https://github.com/elastic/kibana/blob/24534e832e8fb8691d0559a6f29825345f086e09/x-pack/legacy/plugins/lens/public/indexpattern_datasource/datapanel.tsx#L202-L204

@chandlerprall
Copy link
Contributor

Thoughts on the existing setup, some paths we could take, and my opinion at the end

Existing implementation with React components

Relying on React components:

  • re-uses an already-known paradigm
  • provides a guarantee that the configuration happens before its use (EuiContext wraps EuiI18n)
  • changing the configuration propagates to uses without the need for a page reload/refresh

Because of these benefits, we originally opted to use the render props pattern as the sole means of implementing i18n in EUI.

Other ideas

React hook

We've discussed and wanted to add a useI18n hook which would provide the same functionality, but be cleaner. I believe this could be used directly in e.g. aria-label attributes in many or most of our use cases:

<div aria-label={useI18n('euiPopover.screenReaderAnnouncement', 'You are in a dialog. To close this dialog, hit escape.')}>

It would also be usable outside of attributes,

const [button, buttonActive] = useI18n(
  ['euiColumnSorting.button', 'euiColumnSorting.buttonActive']
  ['Sort fields', 'fields sorted']
);

...

return (
  <EuiButtonEmpty
    size="xs"
    iconType="sortable"
    color="text"
    className={controlBtnClasses}
    data-test-subj="dataGridColumnSortingButton"
    onClick={() => setIsOpen(!isOpen)}>
    {numberOfSortedFields > 0
      ? `${numberOfSortedFields} ${buttonActive}`
      : button}
  </EuiButtonEmpty>
);

Drawback:

Hooks cannot be used in logic branches (e.g. if statements, branches), code that would be written as:

return isThing
  ? <a aria-label={useI18n('firstToken', 'some value')} />
  : <button aria-label={useI18n('secondToken', 'some other value')} />

would need to be written as

const [firstToken, secondToken] = useI18n(['firstToken', 'secondToken'], ['some value', 'some other value']);
return isThing
  ? <a aria-label={firstToken} />
  : <button aria-label={secondToken} />
Babel plugin

Instead of a hook, we can add a magic function that looks identical to the hook - it's just function after all - that is converted to the existing EuiI18n structure, e.g.

return isThing
  ? <a aria-label={magicI18n('firstToken', 'some value')} />
  : <button aria-label={magicI18n('secondToken', 'some other value')} />

transparently becomes

return isThing
  ? <EuiI18n token="firstToken" default="some value">{(firstToken) => <a aria-label={firstToken} />}</EuiI18n>
  : <EuiI18n token="secondToken" default="some other value">{(secondToken) => <button aria-label={secondToken} />}</EuiI18n>

Benefits:

Ease of the hooks use without the no-branching requirement

Drawbacks:

  • It's a custom babel plugin to maintain
  • Magic functionality that is specific to EUI, harder for outside contributors to understand
Non-React service

Similar to Kibana's i18n, we could have a service that would need to be configured at some point in the consuming application before any of the strings are rendered/created. This provides the same pattern as the above React hook as it would be callable at any point in time. There are some drawbacks:

  • no automatic re-rendering/translating of strings when localization configuration changes
  • functionality supports calling i18nservice.translate() at any time, but there's no guarantee that happens before the configuration is set which would lead to a never-translated value
  • need to figure out a sensical pattern for combining this config with the existing EuiI18n component & context

Opinion

hidden so you can form your own thoughts first

There are a lot of pros for keeping the functionality tied to React's context & render flows, and the drawbacks on a non-React service are far from insignificant. I strongly recommend avoiding that solution.

I think hooks make more sense in EUI than a magic function. The two drawbacks of hooks are potential bugginess when used conditionally and less readability when needing to use those values conditionally. The bugginess is mitigated by the react-hooks/rules-of-hooks eslint plugin which we already use, but the readability issue will remain.

A magic function + babel plugin solves both the readability & conditionality problems, but strains the mental model and requires effort from consumers if they want to use the magic in their application.

Hooks are well-known and understood within React, and consuming applications could use our hook directly without any additional education or setup. With a magic function, there is some additional documentation/education for consumers who'd also need changes to their babel config to use the same pattern in their app.

@vadistic
Copy link

vadistic commented Jun 18, 2020

Hello - I've just started to work with eui in my project - it's amazing :)

About the hook - you want the hook only to access context - then you can return helpers in any shape you like :)

// 1) conditionals
const {i18n} = useI18n();

return isThing
  ? <a aria-label={i18n('firstToken', 'some value')} />
  : <button aria-label={i18n('secondToken', 'some other value')} />

// 2) or something smarter

const {i18n} = useI18n<TMappingsShape>('myScope'); // firstToken => "myScope.firstToken"  :)

const secondTxt = i18n('secondToken', 'some value')

return isThing
  ? <a aria-label={i18n('firstToken', 'some value')} />
  : <button aria-label={secondTxt} />

// 3) maybe providing some local mappings "from the bottom" ?

const scopedMapping = {
  defaults: {
    localToken: "hello"
  }
}

const {i18n} = useI18n<TGlobalMappingsShape>('scope', scopedMapping);

return  <button aria-label={i18n('localToken')}>{i18n('globalButtons.okMsg')}</button>

Actually, I'm going to play with it a bit in my project - but I have a tiny roadblock - could you consider exporting I18nContext variable?

const I18nContext: React.Context<I18nShape> = createContext({});

Cheers!

@chandlerprall
Copy link
Contributor

could you consider exporting I18nContext variable?

It is, effectively, exported - though if the setup does not support your needs, I'm curious what you're doing :)

The context consumer is exported through EuiI18nConsumer and its provider is wrapped by EuiContext. This split is intentional, to ensure that a "required" context value is provided (or at least known and considered) if/when there is a new EUI context. Essentially, we want any consumer to be usable independently but require all providers be set together.

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 a pull request may close this issue.

4 participants