Skip to content
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

Merged
merged 5 commits into from
Oct 29, 2019

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Oct 21, 2019

Summary

Makes Visualize app use Embeddable API (instead of visualize loader) to show visualization

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch from 09ec4d2 to bee1062 Compare October 22, 2019 11:17
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch from bee1062 to cffdf8a Compare October 22, 2019 12:45
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch from cffdf8a to b0555fa Compare October 22, 2019 14:30
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch from b0555fa to 2321bfb Compare October 23, 2019 06:11
@elasticmachine
Copy link
Contributor

💔 Build Failed

@ppisljar
Copy link
Member Author

retest

@ppisljar
Copy link
Member Author

jenkins, test this

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch 2 times, most recently from 8b301c2 to 90ff0bb Compare October 23, 2019 11:00
@ppisljar ppisljar requested review from lizozom and flash1293 October 23, 2019 11:01
@ppisljar ppisljar added the release_note:skip Skip the PR/issue when compiling release notes label Oct 23, 2019
@ppisljar ppisljar marked this pull request as ready for review October 23, 2019 11:01
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@flash1293 flash1293 left a 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(),
Copy link
Contributor

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 () {
Copy link
Contributor

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 }) {
Copy link
Contributor

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.

Copy link
Member Author

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;
Copy link
Contributor

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

@ppisljar ppisljar force-pushed the visualizeUseEmbeddable branch from 3de1f62 to 904c931 Compare October 24, 2019 11:55
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@flash1293 flash1293 self-requested a review October 29, 2019 08:00
Copy link
Contributor

@flash1293 flash1293 left a 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.

@ppisljar ppisljar merged commit c09e1cd into elastic:master Oct 29, 2019
ppisljar added a commit to ppisljar/kibana that referenced this pull request Oct 29, 2019
@ppisljar ppisljar deleted the visualizeUseEmbeddable branch October 29, 2019 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration release_note:skip Skip the PR/issue when compiling release notes v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants