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 visualize loader for dashboards #15444

Merged
merged 3 commits into from
Dec 8, 2017

Conversation

timroes
Copy link
Contributor

@timroes timroes commented Dec 6, 2017

This PR changes the visualization embeddable for dashboards to use the visualize loader instead of rendering the <visualize> Angular tag itself.

Fixes #15153

@timroes timroes added Feature:Dashboard Dashboard related features Feature:Vis Loader Visualize loader APIs Feature:Visualizations Generic visualization features (in case no more specific feature label is available) chore v6.2.0 v7.0.0 labels Dec 6, 2017
@timroes timroes force-pushed the vizualise-loader-dashboard branch from e85c129 to 49794d1 Compare December 6, 2017 10:49
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some small questions, overall it looks nice.

cssClass: `panel-content ${savedObject.vis.type.name}`,
// The chrome is permanently hidden in "embed mode" in which case we don't want to show the spy pane, since
// we deem that situation to be more public facing and want to hide more detailed information.
showSpyPanel: !chrome.getIsChromePermanentlyHidden(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why this instead of getShouldShowSpyPane() which was use before ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getShouldShowSpyPane returned that value. I talked to Stacey, and there is currently no scenario where this value can actually change while a visualization is visible, so I just replaced it by setting it initially to this value. Also it wouldn't have worked beforehand properly, since changing that permanently hidden value isn't triggering any callback, and is not within the angular scope, so the <visualize> as rendered by dashboards would anyway not have picked up any changes (IF there would be a case where it changes, which doesn't exist).

data-shared-item
data-title="{{sharedItemTitle}}"
data-description="{{savedObj.description}}"
render-counter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is render-counter still used ? if not, why not ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that it's still used. The visualize loader adds this to every visualization (see template). Since this directive has a rather small overhead I thought it's not bad having this on any embedded visualization, and I didn't want to implement a generic param to add angular directives to the <visualize> via the loader, since I considered this worth (regarding possible performance impacts and security).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we still need render-counter for now, for reporting. however, @kobelb has an idea to remove this in favor of a render-state flag. he is looking into it and will put up a proposal.

Copy link
Contributor

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@timroes timroes merged commit eaa94f0 into elastic:master Dec 8, 2017
@timroes timroes deleted the vizualise-loader-dashboard branch December 8, 2017 14:21
timroes added a commit to timroes/kibana that referenced this pull request Dec 8, 2017
* Use visualize loader to in visualize embeddable

* Remove old template

* Fix broken property name
timroes added a commit that referenced this pull request Dec 8, 2017
* Use visualize loader to in visualize embeddable

* Remove old template

* Fix broken property name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Dashboard Dashboard related features Feature:Vis Loader Visualize loader APIs Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:enhancement v6.2.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants