[Lens] Fix embeddable title and description for reporting and dashboard tooltip #78767

dej611 · 2020-09-29T13:31:05Z

Summary

This PR aims to correctly propagate to the Lens embeddable both title and description information to the VisualizationContainer element, used for reporting. ( #70725 )
While in the area, it also addresses rendering differences with Visualize description tooltip behaviour in Dashboard of Lens embeddables. ( #69638 )

Reporting fixes:

Note: the metric visualization was using the metric title as embeddable title, while this PR separate the two things. An idea could be to use the metricTitle as fallback for the title, but I do not see much value in it as metricTitle is already shown.

Dashboard tooltip with description:

Fixes: #70725 and #69638

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2020-09-29T16:00:29Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293

I see why you used the approach in this PR by pushing description and title into the visualization state so it can be piped through the layers, but I'm not sure I like the approach because it magically extends the state which was intended to be private to the visualization.

I propose passing down these things to the toExpression function explicitly by extending the global types:

  toExpression: (
    state: T,
    datasourceLayers: Record<string, DatasourcePublicAPI>
  ) => Ast | string | null;

becomes

  toExpression: (
    state: T,
    datasourceLayers: Record<string, DatasourcePublicAPI>,
    title?: string,
    description?: string
  ) => Ast | string | null;

flash1293 · 2020-09-30T13:13:16Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts

@@ -79,7 +81,7 @@ export async function persistedStateToExpression(

  return buildExpression({
    visualization,
-    visualizationState,
+    visualizationState: { ...(visualizationState as object), title, description },


I'm not sure I like this approach - it assumes a certain state structure we don't guarantee and it seems like it's breaking the current isolation between frame and individual visualizations to magically set state.

dej611 · 2020-09-30T16:34:45Z

Refactored the code. Some toExpression implementations extended the original interface with custom parameter: I tried to refactor that into an attributes parameter which can contain "flags" or options to use in each scenario.

wylieconlon

Tested and LGTM! I included some tests for Lens by Value and found that the titles were not rendered, so I want to loop in @ThomThomson to see if that's expected.

wylieconlon · 2020-09-30T18:34:08Z

src/plugins/embeddable/public/lib/panel/panel_header/panel_header.tsx

 type VisualizeEmbeddable = any;

 function getViewDescription(embeddable: IEmbeddable | VisualizeEmbeddable) {
-  if (embeddable.type === VISUALIZE_EMBEDDABLE_TYPE) {
+  if (embeddable.getVisualizationDescription) {


Could this be embeddable.getDescription instead? Not all embeddables are visualizations

tsullivan · 2020-09-30T18:51:16Z

LGTM, on Reporting integration changes.

ThomThomson

@wylieconlon it seems like the placeholder titles are only missing because master hasn't been merged in since that was added.

Additionally, I agree with on the naming of the getDescription function, and have one comment below

ThomThomson · 2020-09-30T19:52:22Z

src/plugins/embeddable/public/lib/panel/panel_header/panel_header.tsx

@@ -109,11 +109,10 @@ function renderTooltip(description: string) {
  );
 }

-const VISUALIZE_EMBEDDABLE_TYPE = 'visualization';
 type VisualizeEmbeddable = any;


I know this wasn't added in this PR, but I'm not a huge fan of using any like this. Now that this description is added to both lens and visualize, I wonder if it would be worth adding an optional getDescription function to IEmbeddable.

If not, at least the any could be removed with something like:

type EmbeddableWithDescription = IEmbeddable & { getDescription: () => string }; function getViewDescription(embeddable: IEmbeddable) { return (embeddable as EmbeddableWithDescription).getDescription?.() || ''; }

dej611 · 2020-10-01T10:33:07Z

@elasticmachine merge upstream

kibanamachine · 2020-10-01T12:07:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: c4455a3

Metrics [docs]

async chunks size

id	before	after	diff
`lens`	995.6KB	998.6KB	+3.0KB

page load bundle size

id	before	after	diff
`embeddable`	290.6KB	290.5KB	-73.0B
`lens`	139.6KB	139.9KB	+288.0B
`visualizations`	272.6KB	272.6KB	-26.0B
total			+189.0B

History

💛 Build #78645 was flaky 997c58a
💚 Build #78372 succeeded d3b10b5
💛 Build #78194 was flaky 8e8526f
💚 Build #77869 succeeded 92e476e
💔 Build #77856 failed 7f66e63

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

ThomThomson

Thanks for getting rid of that any. New changes LGTM!

Wylie checked it out, it's fine

…rd tooltip (elastic#78767) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…aly-detection-partition-field * 'master' of github.com:elastic/kibana: (76 commits) Fix z-index of KQL Suggestions dropdown (elastic#79184) [babel] remove unused/unneeded babel plugins (elastic#79173) [Search] Fix timeout upgrade link (elastic#79045) Always Show Embeddable Panel Header in Edit Mode (elastic#79152) [Ingest]: add more test for transform index (elastic#79154) [ML] DF Analytics: Collapsable sections on results pages (elastic#76641) [Fleet] Fix agent policy change action migration (elastic#79046) [Ingest Manager] Match package spec `dataset`->`data_stream` and `config_templates`->`policy_templates` renaming (elastic#78699) Revert "[Metrics UI] Add ability to override datafeeds and job config for partition field (elastic#78875)" [ML] Update transform cloning to include description and new fields (elastic#78364) chore(NA): remove non existing plugin paths from case api integration tests (elastic#79127) [Ingest Manager] Ensure we trigger agent policy updated event when we bump revision. (elastic#78836) [Metrics UI] Display No Data context.values as [NO DATA] (elastic#78038) [Monitoring] Missing data alert (elastic#78208) [Lens] Fix embeddable title and description for reporting and dashboard tooltip (elastic#78767) [Lens] Consistent Drag and Drop styles (elastic#78674) [ML] Model management UI fixes and enhancements (elastic#79072) [Metrics UI] Add ability to override datafeeds and job config for partition field (elastic#78875) [Security Solution]Fix basepath used by endpoint telemetry tests (elastic#79027) update rum agent version which contains longtasks (elastic#79105) ...

…ashboard tooltip (#78767) (#79164) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

dej611 added 3 commits September 29, 2020 15:05

🐛 Forward title and description to the visualization container

eb3cd0f

✅ + 📸 Improved tests + updated snapshots

9455c42

🐛 Fix description tooltip for Lens viz in dashboard

7f66e63

dej611 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 Feature:Lens v7.10.0 labels Sep 29, 2020

dej611 self-assigned this Sep 29, 2020

dej611 added 2 commits September 29, 2020 15:50

🐛 Forgotten to commit API change into panel header

fc990e8

🏷️ Fix type checks

92e476e

dej611 marked this pull request as ready for review September 29, 2020 16:00

dej611 requested a review from a team September 29, 2020 16:00

dej611 requested a review from a team as a code owner September 29, 2020 16:00

dej611 requested a review from ThomThomson September 29, 2020 16:16

Merge remote-tracking branch 'upstream/master' into fix/70725

8e8526f

botelastic bot added the Feature:Embedding Embedding content via iFrame label Sep 30, 2020

dej611 added the Feature:Dashboard Dashboard related features label Sep 30, 2020

flash1293 previously requested changes Sep 30, 2020

View reviewed changes

dej611 added 2 commits September 30, 2020 18:28

👌 Refactor based on feedback

31a9d54

✅ Reshaping tests for new implementation

d3b10b5

tsullivan requested review from joelgriffith and tsullivan September 30, 2020 18:31

wylieconlon approved these changes Sep 30, 2020

View reviewed changes

tsullivan approved these changes Sep 30, 2020

View reviewed changes

ThomThomson reviewed Sep 30, 2020

View reviewed changes

👌 Refactor based on feedback

997c58a

dej611 requested a review from flash1293 October 1, 2020 10:10

Merge branch 'master' into fix/70725

c4455a3

ThomThomson approved these changes Oct 1, 2020

View reviewed changes

dej611 merged commit 198c5d9 into elastic:master Oct 1, 2020

dej611 deleted the fix/70725 branch October 1, 2020 16:02

This was referenced Oct 1, 2020

[Lens] Show description in dashboard title #69638

Closed

[7.x] [Lens] Fix embeddable title and description for reporting and dashboard tooltip (#78767) #79164

Merged

dej611 added a commit to dej611/kibana that referenced this pull request Oct 1, 2020

[Lens] Fix embeddable title and description for reporting and dashboa…

68d509c

…rd tooltip (elastic#78767) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

dej611 added a commit that referenced this pull request Oct 2, 2020

[7.x] [Lens] Fix embeddable title and description for reporting and d…

27c412b

…ashboard tooltip (#78767) (#79164) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

nreese mentioned this pull request Oct 2, 2020

Allow all embeddables to display description in Dashboard panel #48359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Fix embeddable title and description for reporting and dashboard tooltip #78767

[Lens] Fix embeddable title and description for reporting and dashboard tooltip #78767

dej611 commented Sep 29, 2020

elasticmachine commented Sep 29, 2020

flash1293 left a comment •

edited

Loading

flash1293 Sep 30, 2020

dej611 commented Sep 30, 2020

wylieconlon left a comment

wylieconlon Sep 30, 2020

tsullivan commented Sep 30, 2020

ThomThomson left a comment •

edited

Loading

ThomThomson Sep 30, 2020 •

edited

Loading

dej611 commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

ThomThomson left a comment

[Lens] Fix embeddable title and description for reporting and dashboard tooltip #78767

[Lens] Fix embeddable title and description for reporting and dashboard tooltip #78767

Conversation

dej611 commented Sep 29, 2020

Summary

Checklist

elasticmachine commented Sep 29, 2020

flash1293 left a comment • edited Loading

Choose a reason for hiding this comment

flash1293 Sep 30, 2020

Choose a reason for hiding this comment

dej611 commented Sep 30, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Sep 30, 2020

Choose a reason for hiding this comment

tsullivan commented Sep 30, 2020

ThomThomson left a comment • edited Loading

Choose a reason for hiding this comment

ThomThomson Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

dej611 commented Oct 1, 2020

kibanamachine commented Oct 1, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

page load bundle size

History

ThomThomson left a comment

Choose a reason for hiding this comment

flash1293 left a comment •

edited

Loading

ThomThomson left a comment •

edited

Loading

ThomThomson Sep 30, 2020 •

edited

Loading