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

Support specific i18n value types in single-token usage #1861

Merged
merged 3 commits into from
Apr 23, 2019

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Apr 19, 2019

Summary

Updates types for EuiI18n for convenience when passing localized values to string-type props, e.g. aria-label. This does not change any functionality, only makes it easier to consume in a common case.

Given the setup,

<EuiI18n token="label" default="Click this button for something to happen.">
  {(label: ReactChild) => <button aria-label={label}></button>}
</EuiI18n>

TS would error passing the ReactChild to aria-label as it cannot downcast ReactChild to a string. It is not possible to type the incoming label as string as the render prop function itself is typed as ReactChild. This PR extends the types' logic for single-token use cases (as above) to retain the default prop's type when calling the render prop function, and the above now works when rewritten to type label as string.

<EuiI18n token="label" default="Click this button for something to happen.">
  {(label: string) => <button aria-label={label}></button>}
</EuiI18n>

The new logic can also infer the type when the default value is a callback,

// works, return from `default` is a string
<EuiI18n token="label" default={() => 'Click this button for something to happen.'}>
  {(label: string) => <button aria-label={label}></button>}
</EuiI18n>

// errors, `default` returns a ReactElement
<EuiI18n token="label" default={() => <p>Click this button for something to happen.</p>}>
  {(label: string) => <button aria-label={label}></button>}
</EuiI18n>

Although unchanged, there are underlying assumptions that any localized, mapped value retains the same type as default. These changes make those assumptions more apparent (see the added type casting and related comments).

This also fails to correctly type if the provided value uses our string interpolation to inject ReactElements. Compare,

// fine, `result` is a string and interpolating it returns a string
<EuiI18n
  token="label"
  values={{ result: 'something to happen' }}
  default="Click this button for {result}."
>
  {(label: string) => <button aria-label={label}></button>}
</EuiI18n>

// doesn't error, but is no longer correct as interpolating `result` returns an array of ReactChilds
<EuiI18n
  token="label"
  values={{ result: <p>something to happen</p> }}
  default="Click this button for {result}."
>
  {(label: string) => <button aria-label={label}></button>}
</EuiI18n>

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

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.

I looked through the examples and understand the usage from a consumer's view. I merged this into #1848 where I was having issues and it now compiles. I am running into an issue I think with VSCode where it still likes to complain and I think this is due to some silly linting bits, but the TS now compiles and I'm assuming my issues are tied to VSCode being noisy. But everything works fine in the actual usage, so this is LGTM.

Here's the goofyness I saw if anyone with VScode wants to check.

image

@thompsongl
Copy link
Contributor

@snide The errors you see look to be related to VSCode. I pulled your branch and merged this branch, and did initially see the 2nd (ReactChild) warning after removing the @ts-ignore comments, but it quickly disappeared (linter plugin caching, perhaps). yarn lint-ts and yarn build both run successfully in that merged environment.

@snide
Copy link
Contributor

snide commented Apr 23, 2019

Yep. I'm OK with this merging. Can update my PR directly after and merge as well.

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Code looks good. As noted, I was able build locally.

No CL entry needed?

@chandlerprall chandlerprall merged commit ed5dee8 into elastic:master Apr 23, 2019
@chandlerprall chandlerprall deleted the update-i18n-typings branch April 23, 2019 16:38
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.

3 participants