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

Convert radio form controls to TS #2438

Merged
merged 5 commits into from
Oct 18, 2019

Conversation

ryankeairns
Copy link
Contributor

@ryankeairns ryankeairns commented Oct 15, 2019

Summary

Converts EuiRadio and EuiRadioGroup to TypeScript.

Note: I attempted to convert the docs example, however it's part of the long 'Form controls' page and I think it may necessitate a setup similar to EuiSelectable to handle the props and Options in the example. Those props pull from a types.tsx that lives with the src component code... I'm speaking out of my depth here, but there are many other form controls which have not been converted in this space and something tells me this may be some sort of solution that rolls these all together. Anyway, given that, I bailed on converting the docs example since this was only for the radio bits.

Checklist

  • Checked in dark mode
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@ryankeairns ryankeairns marked this pull request as ready for review October 15, 2019 21:12
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

The old index.d.ts exported a number of values that index.ts does not. This creates a breaking change, and we should strive to export the meaningful types/interfaces.

Specifically these are now missing from the exports:

  • EuiRadioGroupOption
  • EuiRadioGroupChangeCallback
  • EuiRadioGroupProps
  • EuiRadioProps

src/components/form/radio/radio.tsx Outdated Show resolved Hide resolved
src/components/form/radio/radio.tsx Show resolved Hide resolved
src/components/form/radio/radio_group.tsx Outdated Show resolved Hide resolved
src/components/form/radio/radio_group.tsx Outdated Show resolved Hide resolved
src/components/form/radio/radio_group.tsx Outdated Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

Anyway, given that, I bailed on converting the docs example since this was only for the radio bits.

I think that's the right call, for all the reasons you mentioned 👍

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled and built eui.d.ts locally, tested to ensure expected types are still exported from the @elastic/eui module

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.

LGTM. Kibana type check passes with the generated d.ts

@ryankeairns ryankeairns merged commit d1c3bd1 into elastic:master Oct 18, 2019
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.

None yet

4 participants