Repository navigation
reactifying visualization - #16425
Conversation
df4d11f to
820ffb6
Compare
|
I removed the 6.3 label for now, it's unlikely this'll make it I think. |
820ffb6 to
4902023
Compare
4902023 to
c641470
Compare
c641470 to
7b6ec7f
Compare
997af7c to
cf5b74b
Compare
|
inspector needs to be merged before this can proceed |
30439c6 to
bbb1e50
Compare
bbb1e50 to
e3582df
Compare
7566373 to
b7dd087
Compare
adding visualization loader updating default editor updating visualize updating discover
💚 Build Succeeded |
b7dd087 to
ef2a599
Compare
💚 Build Succeeded |
|
|
||
| export * from './loader'; | ||
| export * from './visualize_loader'; | ||
| export * from './visualization_loader'; |
There was a problem hiding this comment.
Since we don't want to make this a public API that third party plugins consume, could we perhaps not export this via the index.js file, so that we rather import from ui/visualize/loader/visualization_loader, and not having it in the same file that users import the visualize loader from, which we consider public? Maybe even rename that file to _visualization_loader just to make clear, that's nothing you should import outside of visualization code?
timroes
left a comment
There was a problem hiding this comment.
Some minor points.
I would really prefer if we make visualization loader more "invisible" so people not accidentally start using it.
| this.forceUpdate(); | ||
| }; | ||
|
|
||
| static getDerivedStateFromProps(props, prevState) { |
There was a problem hiding this comment.
Could we potentially move the static method to top (or less preferred by me: bottom), to not have it sitting somewhere in the middle?
There was a problem hiding this comment.
i think it makes more sense to order the methods in lifecycle order, but whatever ... this is a real nity pick by the way :)
There was a problem hiding this comment.
I know. That's why I didn't explicitly state it - I though you'll figure it out anyway :D
| })); | ||
| setupAndTeardownInjectorStub(); | ||
|
|
||
| describe('', () => { |
| } | ||
|
|
||
| render() { | ||
| renderPromise = new Promise((resolve) => { |
There was a problem hiding this comment.
We could actually shorten this method to:
async render() {
this.el.innerText = this.vis.params.markdown;
}There was a problem hiding this comment.
but then we won't be able to wait for this renderPromise to be resolved
There was a problem hiding this comment.
That's true. Sorry haven't noticed that! Forget that comment.
|
@timroes we re not hidding any other code we don't want 3rd party developers to use ? i think that's a bad concept. |
|
Yeah I know we don't do that in a lot of places (though we have some places where we use the |
💚 Build Succeeded |
moves from angular to react
this is based on top of #16249 and requires the new inspector panel (spy panel replacement) @timroes is working on before it can be merged
huge thanks to @timroes for helping me out with this 👍