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

EuiDroppable now accepts multiple children (TypeScript) #2634

Merged
merged 1 commit into from
Dec 12, 2019
Merged

EuiDroppable now accepts multiple children (TypeScript) #2634

merged 1 commit into from
Dec 12, 2019

Conversation

seeruk
Copy link
Contributor

@seeruk seeruk commented Dec 11, 2019

Summary

The type definitions for EuiDroppable don't seem to allow multiple children. You can't use examples from the docs, without surrounding multiple children in their own <></> fragment node - this change attempts to remedy that issue.

I'll be honest, I'm not sure if ReactElement[] is the best type for this, I did try to use ReactNode, but that prevented other parts of the type definition from working, whereas ReactElement[] didn't stop anything else from working.

A test case has been added.

Checklist

- [x] Checked in dark mode
- [x] Checked in mobile
- [x] Checked in IE11 and Firefox
- [x] Props have proper autodocs
- [x] Added documentation examples
- [x] Checked for breaking changes and labeled appropriately
- [x] Checked for accessibility including keyboard-only and screenreader modes

  • Added or updated jest tests
  • A changelog entry exists and is marked appropriately

@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?

@snide
Copy link
Contributor

snide commented Dec 11, 2019

Thanks for the contribution! We'll get someone to give a review.

@snide snide requested a review from thompsongl December 11, 2019 21:46
@thompsongl
Copy link
Contributor

jenkins test this

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.

Thanks, @seeruk! ReactElement[] appears to be the correct way to handle this.

@seeruk
Copy link
Contributor Author

seeruk commented Dec 12, 2019

Just updated to resolve the conflict with the changelog.

@thompsongl
Copy link
Contributor

jenkins test this

@thompsongl thompsongl merged commit 3516212 into elastic:master Dec 12, 2019
thompsongl added a commit that referenced this pull request Dec 12, 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.

4 participants