Move DashboardEmptyScreen inside DashboardViewport by majagrubic · Pull Request #51939 · elastic/kibana

majagrubic · 2019-11-30T13:18:54Z

Summary

Intro

As part of the Dashboard-first project, we'll need to add Visualization from the dashboard. I was prototyping this flow, and there was a visible flick where the empty dashboard screen was still shown before the visualization appeared, visible from the GIF below:

What this PR contains?

For the flick to go away, empty screen needs to be inside DashboardViewport. This PR refactors the code so that DashboardEmptyScreen is rendered inside DashboardViewport and gets all the relevant info it needs to render through container input.

What this PR does not contain?

This PR does not actually add visualization from the dashboard. It does not change the layout nor the actions of the empty dashboard screen. It will be added in a subsequent PR for easier review.

How best to review this?

I've add some comments to the relevant places below to help with the review. Some things to look out for: panel rendering, time application, fullscreen mode, report generation.

Checklist

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

This was checked for cross-browser compatibility, including a check against IE11
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

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

majagrubic · 2019-11-30T13:24:17Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

noop for now as we're not in fact adding visualizations yet.

majagrubic · 2019-11-30T13:24:48Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

noop for now, this is where the new logic for adding the visualization will live.

majagrubic · 2019-11-30T13:29:05Z

src/plugins/dashboard_embeddable_container/public/embeddable/viewport/dashboard_viewport.tsx

The empty screen needs to be inside dashboardViewport, but mustn't be inside inside data-shared-items-container or else its content will be included in the report generation. This DOM structure effectively means we could have emptyScreen AND dashboard panels rendered on the screen at the same time, but if the state is always reset appropriately, this should never happen.

Instead of leaving it as a possibility to render both at the same time, should we specifically case the renderings? I wouldn't bet on state always being reset appropriately 😬 (though maybe I'm a pessimist).

majagrubic · 2019-11-30T13:38:13Z

src/plugins/embeddable/public/lib/containers/container.ts

Adding a new panel means it's not an empty state any more, so need to reset.

elasticmachine · 2019-11-30T14:35:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 9b97f09e01000a357c6b028172ec37a6b95d7d3c

elasticmachine · 2019-11-30T18:33:57Z

💔 Build Failed

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

majagrubic · 2019-11-30T18:49:05Z

Jenkins, test this

elasticmachine · 2019-11-30T20:13:11Z

💔 Build Failed

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

majagrubic · 2019-12-01T10:01:30Z

Jenkins, test this

elasticmachine · 2019-12-01T12:05:05Z

💔 Build Failed

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

elasticmachine · 2019-12-06T10:27:49Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 57d44af

majagrubic · 2019-12-06T11:46:09Z

I updated the code again, since the empty state was not being reset properly, which caused the exit fullscreen button to being rendered twice, which was messing the functional test. I updated that one functional test as well, I still don't understand how it was ever passing.

elasticmachine · 2019-12-06T13:01:55Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 0b4a66e

majagrubic · 2019-12-06T15:02:26Z

Jenkins, test this

elasticmachine · 2019-12-06T16:38:15Z

💚 Build Succeeded

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

majagrubic · 2019-12-09T09:33:23Z

@elasticmachine merge upstream

flash1293 · 2019-12-09T10:41:24Z

Not 100% sure whether introduced by this PR, but when the "Use margin between panels" setting is disabled, the light gray background color isn't shown across the full height anymore:

flash1293 · 2019-12-09T10:19:37Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

    }
    $scope.showSaveQuery = dashboardCapabilities.saveQuery as boolean;

+    $scope.getShouldShowEditHelp = () =>


Why are those put on the scope? I can't find a place in angular where they are used

good point. they were on the scope previously, so I just left them there, but it's true that there's no good reason why they should remain there.

flash1293 · 2019-12-09T10:20:52Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

+          dashboardContainer.renderEmpty = () => {
+            const shouldShowEditHelp = $scope.getShouldShowEditHelp();
+            const shouldShowViewHelp = $scope.getShouldShowViewHelp();
+            const isEmptyState = shouldShowEditHelp || shouldShowViewHelp;


It seems like those two functions are always used like this. Would it make sense to put them into a single function and just call it directly here?

No, there is distinction between edit and view screen, it's used in the getEmptyScreenProps to decide which screen should be rendered.

flash1293 · 2019-12-09T10:22:13Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_empty_screen.tsx

-        <h2>{constants.fillDashboardTitle}</h2>
-        {showLinkToVisualize ? addVisualizationParagraph : enterEditModeParagraph}
-      </React.Fragment>
+      <EuiPage className="dshStartScreen" restrictWidth={'36em'}>


Nit: could use restrictWidth="36em"

flash1293 · 2019-12-09T10:37:45Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_app_controller.tsx

        if (!isErrorEmbeddable(container)) {
          dashboardContainer = container;

+          dashboardContainer.renderEmpty = () => {


It feels like we are mixing responsibilities here. From my perspective, the dashboard app and the dashboard container are two separate pieces of code that serve different purposes.

Is it absolutely necessary the container knows about the empty state panel? Couldn't we make that a private detail of the app to keep the interface between the container and the app as small as possible (not injecting the renderEmpty function in a kind of dirty way into the container)? For example we could not render the container at all as long as it's empty and directly render the empty screen from the app itself in line 317:
instead of

container.render(dashboardDom);

we are calling

if (isEmpty) { reactDom.render(<DashboardEmptyScreen {...propsNStuff} />, dashboardDom); } else { container.render(dashboardDom); }

Now if there is an important reason the dashboard container has to know it can be empty, would it make more sense have the empty screen component living in the dashboard container itself instead of the dashboard app? In that scenario the app would just pass in a boolean prop like showEmptyScreen or something like this and the container would take care of the rest.

Please see one of my comments above. Empty screen needs to live inside the dashboardViewport, mainly for the reporting to work properly. data-shared-items container needs to remain where it is. Removing that wrapper will cause the report of empty screen to fail. Putting it around DashboardEmptyScreen means the content of the empty screen will be included in the report.

Now if there is an important reason the dashboard container has to know it can be empty, would it make more sense have the empty screen component living in the dashboard container itself instead of the dashboard app? In that scenario the app would just pass in a boolean prop like showEmptyScreen or something like this and the container would take care of the rest.

I am not quite sure I fully understand this question, but I think there are two things to note:

The props of the empty screen are still unfortunately defined on the dashboard_app_controller: https://github.com/elastic/kibana/pull/51939/files#diff-b2738a4dec941f426bf944b70d057b75R185

DasbhoardContainer is receiving renderEmpty in "a hacky way" as it can't go through input, because all inputs must be serializable

majagrubic · 2019-12-09T11:48:02Z

Re: margins on empty screen, same thing on master:

I'll fix it in one of the next iterations.

flash1293

Synced offline with @majagrubic and I see how the places things are placed ties into the further plans with this, so this LGTM. Nits can be addressed, but it's not blocking the PR.

majagrubic · 2019-12-09T18:37:20Z

thank @flash1293 and @streamich for all the help in getting this done 🙇‍♀ lots more refactoring coming, stay tuned 🎉

elasticmachine · 2019-12-10T13:48:07Z

💚 Build Succeeded

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

History

💚 Build #14130 succeeded 9a128a3

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

cchaos

My comments aren't vital to merging this PR, but something to consider changing now. Otherwise we can do a follow-on PR.

cchaos · 2019-12-10T13:51:28Z

src/legacy/core_plugins/kibana/public/dashboard/dashboard_empty_screen.tsx

+        <EuiPageBody>
+          <EuiPageContent verticalPosition="center" horizontalPosition="center">
+            <EuiIcon type="dashboardApp" size="xxl" color="subdued" />
+            <EuiSpacer size="s" />
+            <EuiText grow={true}>
+              <h2 key={0.5}>{constants.fillDashboardTitle}</h2>
+            </EuiText>
+            <EuiSpacer size="m" />
+            {showLinkToVisualize ? addVisualizationParagraph : enterEditModeParagraph}
+          </EuiPageContent>
+        </EuiPageBody>
+      </EuiPage>


This whole thing can actually be simplified using the EuiEmptyPrompt component. But I can do that in a follow up PR.

This screen is going to change soon, so I would suggest against additional refactoring of it now.

cchaos · 2019-12-10T13:58:55Z

src/plugins/dashboard_embeddable_container/public/embeddable/viewport/dashboard_viewport.tsx

+    const { renderEmpty } = this.props;
+    const { isFullScreenMode } = this.state;
+    return (
+      <div className="dshDashboardEmptyScreen">


This class is only used for tests and has not styles attached to it so its been better practice to use it as a data-test-subj instead. This way if styles were to be added or such, there wouldn't be conflicts between tests and styles.

cchaos · 2019-12-10T14:56:51Z

src/plugins/dashboard_embeddable_container/public/embeddable/viewport/dashboard_viewport.tsx

+    return (
+      <React.Fragment>
+        {this.state.isEmptyState ? this.renderEmptyScreen() : null}
+        {this.renderContainerScreen()}


If you render only the empty state or the container screen, then the split background color problem will be gone.

I'm afraid this needs to stay as is for reporting to work properly for now. But I'm planning on a bigger refactor that will hopefully eliminate this hack then.

cchaos · 2019-12-10T14:59:20Z

src/plugins/dashboard_embeddable_container/public/embeddable/grid/_dashboard_grid.scss

  height: 100% !important; /* 1. */
  width: 100%;
-  position: absolute;
+  position: absolute !important;


The /* 1. */ comment refers to the comment at the top explaining this part, can you also add this comment indicator to the end of this line.

majagrubic · 2019-12-10T15:04:55Z

Thanx @cchaos, will address some of your comments in a follow-up PR.

* Prototyping adding Visualization to Dashboard * i18n fixes * Remvoing dashboard empty screen directive * Updating test for empty dashboard screen * Removing unused state variable * Adding a test for DashboardViewPort * i18n & minor fixes * Fixing fullscreen mode view * Fixing failing functional test (hopefully) * Minor style fix * Fixing EUI text, rendering empty screen OR the panels * Fixing empty screen in fullscreen mode * Update snapshot * Trying to render empty screen from Angular controller * refactor: 💡 don't pass renderEmpty through inputs And make sure isEmptyState is not stale. * Fixing tests after Vadim's commit * Removing unnecessary isEmptyStateProps * Skipping failing test * Removing unnecessary en.json file * Re-adding emptyState, reintroducing functional test * Fixing ja-JP file * Undoing my thing to the functional test

majagrubic commented Nov 30, 2019

View reviewed changes

majagrubic marked this pull request as ready for review November 30, 2019 17:24

majagrubic requested review from a team as code owners November 30, 2019 17:24

majagrubic requested review from a team, myasonik and timroes November 30, 2019 17:25

majagrubic added release_note:skip Skip the PR/issue when compiling release notes v8.0.0 v7.6.0 labels Nov 30, 2019

Maja Grubic added 8 commits December 1, 2019 10:47

Prototyping adding Visualization to Dashboard

de6b965

i18n fixes

1e43993

Remvoing dashboard empty screen directive

4d09d08

Updating test for empty dashboard screen

a9e888b

Removing unused state variable

e7ecbd1

Adding a test for DashboardViewPort

cacfbb0

i18n & minor fixes

e087e1d

Fixing fullscreen mode view

a34c220

majagrubic force-pushed the maja/swithc-to-visualize branch from c7f1ffb to a34c220 Compare December 1, 2019 10:48

Fixing failing functional test (hopefully)

f3e4b32

majagrubic added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Dec 1, 2019

Maja Grubic added 2 commits December 6, 2019 11:16

Removing unnecessary en.json file

94fd3b3

Re-adding emptyState, reintroducing functional test

0b4a66e

majagrubic requested review from flash1293 and removed request for timroes December 6, 2019 11:44

Merge branch 'master' into maja/swithc-to-visualize

b7462bd

Maja Grubic added 2 commits December 6, 2019 15:06

Fixing ja-JP file

74b5ebd

Undoing my thing to the functional test

b5a7cf5

Merge branch 'master' into maja/swithc-to-visualize

9a128a3

flash1293 requested changes Dec 9, 2019

View reviewed changes

flash1293 approved these changes Dec 9, 2019

View reviewed changes

Merge branch 'master' into maja/swithc-to-visualize

c4c7ad9

cchaos approved these changes Dec 10, 2019

View reviewed changes

majagrubic merged commit 717e40c into elastic:master Dec 10, 2019

majagrubic deleted the maja/swithc-to-visualize branch December 10, 2019 15:05

majagrubic mentioned this pull request Dec 10, 2019

[7.x] Move DashboardEmptyScreen inside DashboardViewport (#51939) #52637

Merged

majagrubic mentioned this pull request Dec 10, 2019

[Dashboard] Add visualization from dasbhoard empty screen #52670

Merged

6 tasks

Conversation

majagrubic commented Nov 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Intro

What this PR contains?

What this PR does not contain?

How best to review this?

Checklist

For maintainers

Uh oh!

majagrubic Nov 30, 2019 • 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!

majagrubic Nov 30, 2019 • 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!

majagrubic Nov 30, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Nov 30, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Nov 30, 2019

💔 Build Failed

Uh oh!

majagrubic commented Nov 30, 2019

Uh oh!

elasticmachine commented Nov 30, 2019

💔 Build Failed

Uh oh!

majagrubic commented Dec 1, 2019

Uh oh!

elasticmachine commented Dec 1, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Dec 6, 2019

💔 Build Failed

Uh oh!

majagrubic commented Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 6, 2019

💔 Build Failed

Uh oh!

majagrubic commented Dec 6, 2019

Uh oh!

elasticmachine commented Dec 6, 2019

💚 Build Succeeded

Uh oh!

majagrubic commented Dec 9, 2019

Uh oh!

flash1293 commented Dec 9, 2019

Uh oh!

flash1293 Dec 9, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majagrubic Dec 9, 2019 • 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!

Choose a reason for hiding this comment

majagrubic commented Nov 30, 2019 •

edited

Loading

majagrubic Nov 30, 2019 •

edited

Loading

majagrubic Nov 30, 2019 •

edited

Loading

majagrubic Nov 30, 2019 •

edited

Loading

majagrubic commented Dec 6, 2019 •

edited

Loading

flash1293 Dec 9, 2019 •

edited

Loading

majagrubic Dec 9, 2019 •

edited

Loading

majagrubic commented Dec 9, 2019 •

edited

Loading

flash1293 left a comment •

edited

Loading

majagrubic commented Dec 9, 2019 •

edited

Loading