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

EuiGlobalToastListItem convert to TS #1880

Merged
merged 4 commits into from
Apr 29, 2019

Conversation

theodesp
Copy link
Contributor

Summary

Converted EuiGlobalToastListItem to TS

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

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

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.

Given the requests in global_toast_list_item.tsx, my read is that we should restrict its children to EuiToast components and make the prop required. I'd like @chandlerprall's input on that decision, though

src/components/toast/index.d.ts Outdated Show resolved Hide resolved
src/components/toast/global_toast_list_item.tsx Outdated Show resolved Hide resolved
src/components/toast/global_toast_list_item.tsx Outdated Show resolved Hide resolved
@chandlerprall
Copy link
Contributor

Given the requests in global_toast_list_item.tsx, my read is that we should restrict its children to EuiToast components and make the prop required.

I agree, but I've been having difficulty getting that kind of typing against children to work (spent a few hours yesterday doing a similar thing with #1879). I'll keep playing with it, as that would be a very powerful tool for us to have available.

@chandlerprall
Copy link
Contributor

TypeScript does not support restricting children beyond ReactElement (microsoft/TypeScript#21699). Until then, use ReactElement and we'll track this occurrence in #1890

@theodesp theodesp force-pushed the task/EuiGlobalToastListItem-to-TS branch from 0146e81 to 9c72666 Compare April 29, 2019 13:28
@theodesp
Copy link
Contributor Author

@thompsongl PR updated Ta

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.

Good to go after this last change

src/components/toast/index.d.ts Outdated Show resolved Hide resolved
@thompsongl thompsongl merged commit 86b5e64 into elastic:master Apr 29, 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.

5 participants