[Dashboard][Embeddable] Add placeholder title to visualize embeddable by majagrubic · Pull Request #74960 · elastic/kibana

majagrubic · 2020-08-13T15:17:08Z

Summary

This is something I've been meaning to implement for a while - a placeholder title for when visualization is "by-value" mode.
This is how it looks like:

When adding title as usual, title will be shown.

Things get a bit more complicated when saving/unsaving from library. Could you have a look and see if this is as you expect it to be @ryankeairns ?

NOTE: Visualizations by value are not enabled by default. To test this PR, you'll need to manually change the config setting to true here: https://github.com/elastic/kibana/blob/master/src/plugins/dashboard/config.ts#L23

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

ryankeairns · 2020-08-13T15:53:33Z

This is looking good!

One potential issue I noticed is that the title gets erased when toggling the 'Show panel title' switch off

Steps to reproduce:

Add a viz panel with no title
Edit the panel title, type in a new title, click Save in modal (title displays as expected), then Save the Dashboard
Switch back to dashboard Edit mode, Edit the panel title, switch 'Show panel title' to off, click Save in modal

Expected result
The title is visually hidden
Opening the 'Edit panel title' modal shows the swtich off, and the title is still present (disabled)

Actual result
[No title] is displayed
Opening the 'Edit panel title' modal shows the swtich is off, but the title is now empty

majagrubic · 2020-08-14T13:54:07Z

ok, this is a bit more convoluted than I thought. will work on it in the next couple of days

…itle

elasticmachine · 2020-09-15T12:37:38Z

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

ryankeairns · 2020-09-15T16:20:34Z

@majagrubic I might be confusing myself, but when I create a new viz from Dashboard, shouldn't this PR allow me to save without adding a title? I just tried this again and it seems the title is now required.

majagrubic · 2020-09-16T07:30:48Z

@ryankeairns by default we don't allow saving embeddables by value just yet. you'll need to manually change the config value to true when testing this PR here: https://github.com/elastic/kibana/blob/master/src/plugins/dashboard/config.ts#L23

ryankeairns · 2020-09-16T14:44:44Z

This is looking good!

One potential issue I noticed is that the title gets erased when toggling the 'Show panel title' switch off

Steps to reproduce:
* Add a viz panel with no title

* Edit the panel title, type in a new title, click Save in modal (title displays as expected), then Save the Dashboard

* Switch back to dashboard Edit mode, Edit the panel title, switch 'Show panel title' to _off_, click Save in modal
Expected result
The title is visually hidden
Opening the 'Edit panel title' modal shows the swtich off, and the title is still present (disabled)

Actual result
[No title] is displayed
Opening the 'Edit panel title' modal shows the swtich is off, but the title is now empty

Thanks for looking into this further. The title is now visually hidden as opposed to showing [No title], which is good, however the value is still being erased. It's not a huge deal, but when I choose to show/hide the title, I expect the value to remain.

When I initially toggle it off, the value is still present, just disabled

When I return to toggle it back on, the value is gone

ryankeairns · 2020-09-16T19:51:48Z

I've been thinking about this more and agree that it is convoluted. Here is what I'm expecting to happen, but perhaps it can be simplified:

	Panel title field is empty	Panel title field has a value
Show panel title is on	Display [No title]	Display actual title
Show panel title is off	Display nothing	Display nothing*

*The bottom right box is where I see the issue. I had previously entered a title, then decided to toggle Show panel title off. It doesn't mean I wanted to delete/reset the Panel title field, I just wanted to hide it from displaying.

majagrubic · 2020-09-17T16:20:02Z

@ryankeairns thanx for the detailed description of the flow. I had a look at this today and supporting this last use-case is quite tricky, due to the architecture of embeddables (I understand it sounds super simple). Can we leave it for now as-is and leave it as an improvement at a later stage?

ryankeairns · 2020-09-17T19:02:06Z

@majagrubic sure, please just create an issue to track the follow up work. Thank you.

majagrubic · 2020-09-18T09:40:00Z

@elasticmachine merge upstream

majagrubic · 2020-09-18T09:45:17Z

Created a follow-up issue: #77862

ThomThomson

Tested locally on chrome,

I really like how by value mode makes the 'show panel title' switch default to false. The addition of the placeholder title also makes a lot of sense and contributes nicely to rounding out our 'by value' features!

Second look at code

ThomThomson

Upon looking at the code again, I am realizing that there are a few ways to simplify this greatly and minimize the impact on other embeddables / places around the code base. It should be possible to implement this entirely in panel_header.tsx.

ThomThomson · 2020-09-18T20:50:51Z

src/plugins/embeddable/public/lib/embeddables/i_embeddable.ts

+  /**
+   * Returns the placeholder title of this embeddable, if it exists.
+   */
+  getPlaceholderTitle?(): string | undefined;


Seems to me that the placeholder title shouldn't go in a public API like this. Why should each embeddable know about the placeholder title?

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

ThomThomson · 2020-09-18T20:58:55Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+    // propagate the placeholder title to the output embeddable
+    // but only when the visualization is in edit/Visualize mode
+    if (!this.parent && this.placeholderTitle !== this.output.placeholderTitle) {
+      this.updateOutput({ placeholderTitle: this.placeholderTitle });


Would this mean that every embeddable is required to propagate the placeholder title in order for it to work?

Not sure what you mean by "every embeddable", but if you mean "every visualize embeddable that had its placeholder title changed", then the answer is yes 😄

Oh I see, I thought that this was required to propagate the dashboard placeholder title to the embeddable output so that the panel header could pick it up. The lens embeddable for instance, doesn't show the placeholder title.

It sounds like this is to support custom overrides to the placeholder title by embeddables?

ThomThomson · 2020-09-18T21:15:07Z

src/plugins/dashboard/public/application/dashboard_app_controller.tsx

                // @ts-expect-error
                delete input.id;
+                const placeholderTitle = i18n.translate('dashboard.embeddable.placeholderTitle', {
+                  defaultMessage: '[No Title]',


Is there a use case for defining this in the dashboard controller? Maybe there's something I'm missing, but couldn't this be defined in the panel header so it wouldn't have to passed around so much?

Dashboard "decides" what the placeholder title for embeddables on the dashboard will be. Moving this inside panel header would mean that placeholder title for every embeddable would need to be the same. Additionally, we're polluting that component by giving it more responsibility than it should have. That component should render the title regardless of where it came from.

I see what you mean, but I am still more of the opinion that a placeholder title is exactly the kind of responsibility that the header should have.

It also seems like giving each panel header the same placeholder title would result in greater consistency, and that an addition to i_embeddable is a much more polluting change than adding functionality to panel_header

majagrubic · 2020-09-21T06:54:35Z

@elasticmachine merge upstream

kibanamachine · 2020-09-23T11:00:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 903dd6b

Build metrics

async chunks size

id	value	diff	baseline
visualize	414.8KB	+28.0B	414.7KB

page load bundle size

id	value	diff	baseline
dashboard	708.4KB	+26.0B	708.3KB
embeddable	432.5KB	+2.3KB	430.2KB
total		+2.3KB

History

💔 Build #76448 failed 04d8007
💚 Build #76135 succeeded bc8af33
💚 Build #75772 succeeded d0fb3cd
💚 Build #75480 succeeded d8601a3
💚 Build #74408 succeeded 66ea7d9

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

majagrubic · 2020-09-28T12:26:03Z

@elasticmachine merge upstream

elasticmachine · 2020-09-28T12:26:05Z

merge conflict between base and head

AlonaNadler · 2020-09-29T20:42:30Z

I can't pull the pr ATM but a few points I notice from the conversation :

title for visualizations is important. In case someone didn't add a title I think you should show no title when on edit mode. On reading mode does it also show no title? if users didn't add title on reading mode can it appear empty? it won't be friendly if all panels will show add title
Ideally we had an easier way proximate to the title itself to change the title @majagrubic

[Dashboard][Embeddable] Add placeholder title to visualize embeddable

3669a86

majagrubic requested a review from ryankeairns August 13, 2020 15:23

Maja Grubic added 6 commits August 17, 2020 09:18

Trying to add placeholder title to embeddable

3e1e8c1

Merge master

9043ac7

Merge branch 'master' into add-title-to-vis-emebeddable

937a7a9

Fixing issue with placeholder title not disappearing after hiding a t…

38768c4

…itle

Fix failing jest test

269b636

Merge master

66ea7d9

majagrubic marked this pull request as ready for review September 15, 2020 12:35

majagrubic requested a review from a team September 15, 2020 12:35

majagrubic requested review from a team as code owners September 15, 2020 12:35

majagrubic added release_note:roadmap Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Sep 15, 2020

majagrubic added the v7.10.0 label Sep 15, 2020

majagrubic mentioned this pull request Sep 17, 2020

[Visualize Embeddable] Persisting title on vis #77706

Closed

3 tasks

Merge branch 'master' into add-title-to-vis-emebeddable

d8601a3

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

ThomThomson previously approved these changes Sep 18, 2020

View reviewed changes

ThomThomson self-requested a review September 18, 2020 20:40

ThomThomson reviewed Sep 18, 2020

View reviewed changes

elasticmachine and others added 5 commits September 21, 2020 02:54

Merge branch 'master' into add-title-to-vis-emebeddable

d0fb3cd

Merge branch 'master' into add-title-to-vis-emebeddable

bc8af33

Refactor panel header a bit

d7887e5

Use placeholderTitle on panel header, not dashboard controller

933f8b2

Making typescript happy

04d8007

majagrubic requested a review from ThomThomson September 23, 2020 08:48

Fixin i18n

903dd6b

ThomThomson mentioned this pull request Sep 23, 2020

[Time to Visualize] Panel Title Fixes #78365

Merged

3 tasks

majagrubic closed this Oct 2, 2020

Conversation

majagrubic commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

ryankeairns commented Aug 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majagrubic commented Aug 14, 2020

Uh oh!

elasticmachine commented Sep 15, 2020

Uh oh!

ryankeairns commented Sep 15, 2020

Uh oh!

majagrubic commented Sep 16, 2020

Uh oh!

ryankeairns commented Sep 16, 2020

When I initially toggle it off, the value is still present, just disabled

When I return to toggle it back on, the value is gone

Uh oh!

ryankeairns commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majagrubic commented Sep 17, 2020

Uh oh!

ryankeairns commented Sep 17, 2020

Uh oh!

majagrubic commented Sep 18, 2020

Uh oh!

majagrubic commented Sep 18, 2020

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson left a comment

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ThomThomson Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 18, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomThomson Sep 21, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic commented Sep 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Sep 23, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

History

Uh oh!

majagrubic commented Sep 28, 2020

Uh oh!

elasticmachine commented Sep 28, 2020

Uh oh!

AlonaNadler commented Sep 29, 2020

Uh oh!

Reviewers

majagrubic commented Aug 13, 2020 •

edited

Loading

ryankeairns commented Aug 13, 2020 •

edited

Loading

ryankeairns commented Sep 16, 2020 •

edited

Loading

ThomThomson Sep 21, 2020 •

edited

Loading

majagrubic Sep 21, 2020 •

edited

Loading

majagrubic commented Sep 21, 2020 •

edited

Loading