-
Notifications
You must be signed in to change notification settings - Fork 842
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
uses React.RefCallback instead of custom implementation #2929
Conversation
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do add a changelog entry for this 👍
Thanks for following up, @dimitropoulos !
Oh, cool! To extend Greg's changelog ask, this should be marked as a breaking change as it forces consumers to update their @types/ packages, likely requiring syncing those definitions in yarn's & npm's lock files. Let's hold off on merging this until we get the existing EUI release (v20.0.0) fixed up and on a merging path in Kibana. |
c272ba7
to
4ea7609
Compare
Cool stuff. Yeah, no hurry in the slightest - I mostly just didn't want to forget! haha. Take your time and merge this whenever is appropriate 😸 I updated the changelog in 6d917c1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after conflict resolution
We'll wait for Chandler's ok before merging
This is clear to merge! |
jenkins test this |
Fixed the conflicts just to speed this one up so we can merge. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2929/ |
@snide thanks! I just got out of work so I wasn't able to come rebase it until now. looks good! |
Fixed CL since the last release. Will merge on green. This will be a breaking change. |
jenkins test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_2929/ |
This is a followup to #2838 (comment) where I found the need for a RefCallback type that matches React's type implementation. I made the change in the react types, and now that that's been released this PR just serves to finish the job by updating this project to use the React.RefCallback type.
Since this is internal-only and really a quite minor and very non-breaking change I didn't add to the CHANGELOG but please let me know if it's still appropriate to do so in the case of an update like this.
Thanks!