Repository navigation
moving renderers registry to OSS - #28986
Conversation
💚 Build Succeeded |
w33ble
left a comment
There was a problem hiding this comment.
It looks like you need to clean up some code in Canvas still.
| @@ -0,0 +1,42 @@ | |||
| /* | |||
There was a problem hiding this comment.
It looks like you just moved kibana/x-pack/plugins/canvas/public/lib/render_function.js here, but you didn't remove the source. The pr says moving the registry, but it looks like you just copied it and didn't replace the old use...
| @@ -0,0 +1,29 @@ | |||
| /* | |||
There was a problem hiding this comment.
Same as render_functions.js, this code is now duplicated here and in the Canvas code.
There was a problem hiding this comment.
thanks Joe, seems i messed up my commit ... i wanted to move files from canvas, but seems i copied them.
💚 Build Succeeded |
lukeelmers
left a comment
There was a problem hiding this comment.
Code LGTM, tested locally as well & no obvious issues.
monfera
left a comment
There was a problem hiding this comment.
Code looks great! The inclusion of renderers in the (outer) types object is a good move anyway. It worked with some fairly complex pipelines and the Flights / sales / logs examples in Canvas.
|
Thoughts on separating the renderers registry out from the kbn-interpreter package? Feels like they could be separate concerns. One handles rendering data of a given shape, the other executes an expression. A plugin, I think, could technically want to use only the interpreter but have no desire to render the data (maybe just passing it on to something else... or for example if we create the idea of executing action expression - nothing to render at the end). |
|
currently they all get loaded with the same plugin system (which code is inside interpreter package) |
Summary
Moves
renderersregistry to OSS, as we will use it in visualize as well. All current renderers are still in canvas. This will allow visualize to register its own renderer, which will make using all visualizations inside canvas possible.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.[ ] This was checked for cross-browser compatibility, including a check against IE11[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support[ ] Documentation was added for features that require explanation or tutorials[ ] Unit or functional tests were updated or added to match the most common scenarios[ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
[ ] This was checked for breaking API changes and was labeled appropriately[ ] This includes a feature addition or change that requires a release note and was labeled appropriately