-
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
use embeddables in visualize editor #48744
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
💔 Build Failed
|
09ec4d2
to
bee1062
Compare
💔 Build Failed
|
bee1062
to
cffdf8a
Compare
💔 Build Failed
|
cffdf8a
to
b0555fa
Compare
💔 Build Failed
|
b0555fa
to
2321bfb
Compare
💔 Build Failed
|
retest |
jenkins, test this |
8b301c2
to
90ff0bb
Compare
💔 Build Failed
|
💚 Build Succeeded
|
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.
Hitting the refresh button does not trigger a refresh for me. This might be unrelated, but clicking a pie slice results in a console error (something about esQuery)
$scope.renderFunction = async () => { | ||
if (!$scope._handler) { | ||
$scope._handler = await embeddables.getEmbeddableFactory('visualization').createFromObject($scope.savedObj, { | ||
//vis: $scope.savedObj.vis.getUiState(), |
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.
nit: commented out code
@@ -101,7 +101,7 @@ export default function ({ getService, getPageObjects }) { | |||
expect(rowData).to.have.string('Sep 21, 2015 @ 11:59:22.316'); | |||
}); | |||
|
|||
it('should modify the time range when the histogram is brushed', async function () { | |||
it.skip('should modify the time range when the histogram is brushed', async function () { |
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.
I think this got fixed on master
@@ -57,7 +58,7 @@ const defaultEditor = function ($rootScope, $compile) { | |||
} | |||
} | |||
|
|||
render({ uiState, timeRange, filters, appState }) { | |||
render({ uiState, timeRange, filters, query }) { |
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.
what's the difference? Relying on getAppState
instead of using the one that got passed in seems like a step backwards into angular code.
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 need to get rid of appState all together, so it doesn't make much difference here.
@@ -69,6 +73,7 @@ export interface VisualizeOutput extends EmbeddableOutput { | |||
export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOutput> { | |||
private savedVisualization: VisSavedObject; | |||
private loader: VisualizeLoader; | |||
private appState: 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.
Looks like this is PersistedState | undefined
3de1f62
to
904c931
Compare
💚 Build Succeeded |
💚 Build Succeeded |
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.
Tested and works, code LGTM.
Summary
Makes Visualize app use Embeddable API (instead of visualize loader) to show visualization
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers