Embeddable examples on the platform and included with --run-examples flag#52111
Conversation
5331cc3 to
1363ae0
Compare
1363ae0 to
80c2e00
Compare
--run-examples flag
0285490 to
77f5eb2
Compare
041e4d3 to
ad20043
Compare
d84bd5c to
86b893f
Compare
There was a problem hiding this comment.
cc @streamich -- am adding this helper react class. All my examples ended up with the same subscription code, etc. Lmk if you have any suggestions.
There was a problem hiding this comment.
This withEmbeddableSubscription higher order component seems to be OK. I'm planning to improve Embeddable DX in general for React, so you would not need to write such helpers.
5c92648 to
a3fe7c1
Compare
a3fe7c1 to
3992a12
Compare
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
3992a12 to
68741f3
Compare
|
@elasticmachine merge upstream |
lizozom
left a comment
There was a problem hiding this comment.
Added a couple of UX oriented comments.
Overall LGTM
There was a problem hiding this comment.
yea, there is a /* eslint-disable */ surrounding it (as there was originally). The example plugins actually make this easier because they can export the examples at the top level, while we don't want that in here. I've thought about replacing all these samples with ones from the examples repo but some really are only built for testing not so much examples. Some of this could still probably be cleaned up and more example embeddables used for testing.
There was a problem hiding this comment.
Why would you register some in the setup phase and dome in the start?
I assume they internally consume services available only in the start phase, but how can we make this clearer to devs (including ourselves)?
There was a problem hiding this comment.
Yep, because deps.embeddable.getEmbeddableFactory is only available in start, not setup. :( I could move them all to start... or make getEmbeddableFactory available in both. I'm not sure what the best thing is, I think this whole when to expose a registry getter we still don't have a super clear recommendation of. Unless we do and I am just not caught up yet. For now I'll add a comment.
There was a problem hiding this comment.
I find the fact that filter applies only to inner items to be confusing
If you could hide empty items or gray them out, it would be much clearer.
There was a problem hiding this comment.
Yea, I agree, but I wanted an example of a container passing data down to the child. I agree the scenario you mention would make more sense but we'd need them to implement something so the child could communicate "i have a match vs no match" to it's parent and we don't have that kind of communication yet.
Think of it like on the dashboard - we pass down the filters and the child does something with that, but the container never changes how the child is rendered.
It is something to think about though. I wonder if there is a better example I could do here. Maybe pass down "highlight" and highlight the search terms instead of filter by them? What do you think?
There was a problem hiding this comment.
Actually that's a great idea - search highlight would make it much clearer
There was a problem hiding this comment.
Couldn't EmbeddableFactoryRenderer and EmbeddableRoot be the same component?
It feels like they couldn't really exist one without the other, or could they?
There was a problem hiding this comment.
EmbeddableFactoryRenderer encapsulates more logic, but EmbeddableRoot is helpful if you need a reference to the embeddable object, like in todo_embeddable_example. Adding some text there:
You may also notice this example uses `EmbeddableRoot` instead of the
`EmbeddableFactoryRenderer` helper component. This is because it needs a reference
to the embeddable to update it, and `EmbeddableFactoryRenderer` creates and holds
the embeddable instance internally.
examples/embeddable_explorer/public/hello_world_embeddable_example.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
If it's a TODO app, I would expect to be able to add new items.
I could't figure out how to add a new item, just edit the existing one?
There was a problem hiding this comment.
Not yet, but it's a good idea to add and I might be able to showcase the add panel flyout. Just felt like this PR was getting big so wanted to stop. There are def more examples of use cases I'd like to add.
c725678 to
f51ffa8
Compare
|
@lizozom - I updated the examples so that the parent does actually filter out the children (I realized the current infrastructure will actually support this). I also added highlighting. This change of code actually helped me realize some bugs:
|
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…` flag (elastic#52111) * Add a new platform embeddable example plugin * Remove extra hello world test impl. * cleanup * code review updates * Change example to highlight and have parent filter out children * Fix deep comparison of embeddable prop * adjust help text

Embeddable examples built and tested on the platform
Builds off #52027 and adds some new embeddable examples that are in the new platform. Also adds more text to make it more helpful, so it's not just for testing purposes but built to help developers as well.
Run with
yarn start --run-examplesand navigate to theEmbeddable explorerto play around.In addition to the examples, some code changes were made as well:
EmbeddableRootreact helper component that takes care of rendering the embeddable on a react root node.EmbeddableFactoryRendererreact helper component that takes care of getting the right factory, and instantiating a new embeddable with the given input, and then mounting and rendering it to the DOM.withEmbeddableSubscriptionreact helper HOC which takes care of the inner react component binding itself to state changes from the embeddable object.Some extra clean up:
More things to follow:
EmbeddableFactoryRendereris not type safe in regard to the input shape and the type of embeddable. Adding more type safety, similar to how the data search services does it would be great. Filed: [Embeddables] Extra type safety #52887fyi @streamich @oatkiller