-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Embeddable] Add seamless React integration #143131
Conversation
87dd283
to
895ed5a
Compare
Pinging @elastic/kibana-app-services (Team:AppServicesUx) |
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.
VisEditors changes LGTM, everything still works fine for the visualize embeddable
f47f42b
to
e04ff71
Compare
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.
Code LGTM, also checked dashboards on Mac/Chrome all seems to work.
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.
Looked through the code and left one nit / question, but otherwise this is looking great! Nicely done, and super exciting for the future of embeddables.
I also ran the storybooks and everything checks out there.
One problem I ran into, I think this PR introduces a regression in Control Error state, where it doesn't appear when running onFatalError
. To test this I added this.onFatalError(new Error('FATAL ERROR TEST'));
under line 167 of src/plugins/controls/public/options_list/embeddable/options_list_embeddable.tsx
.
but on this branch no error is shown at all. I debugged a little bit, and found that the control frame subscription is receiving the error properly, but I didn't look into it further than that.
Will approve once the error state is fixed.
@@ -33,7 +33,8 @@ export interface EmbeddableOutput { | |||
|
|||
export interface IEmbeddable< | |||
I extends EmbeddableInput = EmbeddableInput, | |||
O extends EmbeddableOutput = EmbeddableOutput | |||
O extends EmbeddableOutput = EmbeddableOutput, | |||
N = any |
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.
Any way to get better typing here? Maybe it extends ReactNode
?
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.
We cannot extend this type parameter from the ReactNode
as I wanted to keep the base embeddable class framework agnostic. In that case, the rendering component could specify what kind of rendering it supports.
The unknown
type (so as never
) is not going to work here as it breaks the existing code. The only option to achieve the same without touching already existing embeddable is using any
.
e8590e2
to
b617a68
Compare
@ThomThomson It is fixed and works as before. Could you please take another look? |
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.
Re-reviewed this locally, and everything LGTM now! Thanks for fixing the error state.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: |
Summary
Resolves #140395.
Checklist