Streamline adding a visualization to a dashboard by stacey-gammon · Pull Request #9400 · elastic/kibana

stacey-gammon · 2016-12-07T18:10:19Z

Implements #9554.

kobelb

I really like this new workflow, just a few minor comments/suggestions.

kobelb · 2017-01-02T16:41:38Z

src/core_plugins/kibana/public/dashboard/index.js

Using $location.search here is creating an entry in the history, which is going to cause the back-button to behave weirdly.

It looks like there's https://code.angularjs.org/1.4.7/docs/api/ng/service/$location#replace we can use, but it applies to all changes during the current digest cycle, which might not be what we want.

I think replace does the right thing, nothing else is updating the url like I am so it'll just reset on the next url change.

kobelb · 2017-01-02T16:49:57Z

src/core_plugins/kibana/public/visualize/editor/editor.js

Same history issue as mentioned before.

updated (good catch on this)

kobelb · 2017-01-02T16:50:52Z

src/core_plugins/kibana/public/visualize/editor/editor.js

I think it'd help readability to move this to right before we replace the parameter in the URL.

kobelb · 2017-01-02T18:26:17Z

src/core_plugins/kibana/public/visualize/editor/editor.js

Any reason we aren't using the kbnUrl.change here instead?

I added a comment, it's because the dashboard url is a full url. Thought it was safer to just redirect there, rather than add something to detect the right suffix to use, which would be fragile to any url prefix changes.

kobelb · 2017-01-02T18:48:17Z

src/core_plugins/kibana/public/visualize/wizard/wizard.js

Same $location.search issue as above.

… add again and again Move the link into the saved object finder and get rid of the management link fix crash in tests lint fixes

make the function simpler and reduce the need to pass null everywhere

kobelb

LGTM

cjcenizal

Functionally, this looks awesome! I just have a few suggestions.

cjcenizal · 2017-01-02T22:23:31Z

src/core_plugins/kibana/public/dashboard/index.js

+        kbnUrl.removeParam(DashboardConsts.ADD_VIS_PARAM);
+      }
+
+      $scope.addNewVis = function () {


Might as well make this a named function so it's easier to identify in the call stack:

$scope.addNewVis = function addNewVis() {

And does this need to be on the $scope? It doesn't seem like it does.

cjcenizal · 2017-01-02T22:31:32Z

src/core_plugins/kibana/public/visualize/editor/editor.js

+          // Not using kbnUrl.change here because the dashboardBaseUrl is a full path, not a url suffix.
+          // Rather than guess the right substring, we'll just navigate there directly, just as if the user
+          // clicked the dashboard link in the UI.
+          window.location.href = `${dashboardBaseUrl.lastSubUrl}&${DashboardConsts.ADD_VIS_PARAM}=${savedVis.id}`;


I think we want to use $window instead of window, so we can mock it in unit tests.

cjcenizal · 2017-01-02T22:35:05Z

src/core_plugins/kibana/public/visualize/wizard/wizard.js

+module.controller('VisualizeWizardStep1', function ($scope, $route, kbnUrl, timefilter, Private) {
  timefilter.enabled = false;

+  $scope.addToDashMode = $route.current.params[DashboardConsts.ADD_TO_DASH_PARAM];


If the template doesn't need access to addToDashMode then this should just be a const instead of a scope variable.

cjcenizal · 2017-01-02T22:35:28Z

src/core_plugins/kibana/public/visualize/wizard/wizard.js

-module.controller('VisualizeWizardStep2', function ($route, $scope, $location, timefilter, kbnUrl) {
+module.controller('VisualizeWizardStep2', function ($route, $scope, timefilter, kbnUrl) {
  const type = $route.current.params.type;
+  $scope.addToDashMode = $route.current.params[DashboardConsts.ADD_TO_DASH_PARAM];


If the template doesn't need access to addToDashMode then this should just be a const instead of a scope variable.

cjcenizal · 2017-01-02T22:36:23Z

src/core_plugins/kibana/public/visualize/editor/panels/save.html

    class="btn btn-primary"
  >
-    Save
+    {{ opts.isAddToDashMode() ? 'Save and Return' : 'Save'}}


Can we make this a little more explicit by changing it to 'Save and Add to Dashboard'?

cjcenizal · 2017-01-02T22:43:26Z

src/core_plugins/kibana/public/dashboard/dashboard_consts.js

+
+export const DashboardConsts = {
+  ADD_TO_DASH_PARAM: 'addToDashboard',
+  ADD_VIS_PARAM: 'addVisualization'


I'm having a hard time telling these two apart because the names are so similar.

I think these would be a little clearer if they were ADD_VISUALIZATION_TO_DASHBOARD_MODE_PARAM and NEW_VISUALIZATION_ID_PARAM. I know this seems verbose, but that's only because our nouns happen to have lots of letters in them -- in terms of using as few words as possible to communicate the maximum amount of information, I think they're pretty efficient!

cjcenizal · 2017-01-02T22:44:25Z

src/core_plugins/kibana/public/dashboard/index.js

 import indexTemplate from 'plugins/kibana/dashboard/index.html';
 import { savedDashboardRegister } from 'plugins/kibana/dashboard/services/saved_dashboard_register';
 import { createPanelState } from 'plugins/kibana/dashboard/components/panel/lib/panel_state';
+import { DashboardConsts } from './dashboard_consts';


Can we name the file and variable without abbreviating "constants"? e.g. DashboardConstants and ./dashboard_constants. There's not much gained by abbreviating them and "const" is a keyword that I think is worth differentiating from the concept of "constants".

cjcenizal · 2017-01-02T22:46:59Z

src/core_plugins/kibana/public/visualize/editor/editor.js

+    $scope.addToDashMode = $route.current.params[DashboardConsts.ADD_TO_DASH_PARAM];
+    kbnUrl.removeParam(DashboardConsts.ADD_TO_DASH_PARAM);
+
+    $scope.isAddToDashMode = () => { return $scope.addToDashMode; };


This can be simplified:

$scope.isAddToDashMode = () => $scope.addToDashMode;

cjcenizal · 2017-01-02T22:48:47Z

src/ui/public/url/url.js

+   */
+  self.removeParam = function (param) {
+    $location.search(param, null).replace();
+  };


Can you add unit tests for this?

stacey-gammon · 2017-01-03T16:00:30Z

@cjcenizal thanks for the comments and feedback, should be all addressed!

cjcenizal

LGTM!

kobelb · 2017-01-03T16:26:44Z

FWIW, the changes look good to me as well

Backports PR #9400 **Commit 1:** Fix bug with param sticking around in the url causing multiple vis to add again and again Move the link into the saved object finder and get rid of the management link fix crash in tests lint fixes * Original sha: 4ff6030 * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-07T17:46:05Z **Commit 2:** rebase with master * Original sha: df8142d * Authored by Stacey Gammon <gammon@elastic.co> on 2016-12-15T19:02:05Z **Commit 3:** code review comments make the function simpler and reduce the need to pass null everywhere * Original sha: 0dd98e7 * Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-02T19:31:37Z **Commit 4:** address comments and cleanup * Original sha: d18f14c * Authored by Stacey Gammon <gammon@elastic.co> on 2017-01-03T15:58:19Z

* Fix bug with param sticking around in the url causing multiple vis to add again and again Move the link into the saved object finder and get rid of the management link fix crash in tests lint fixes * rebase with master * code review comments make the function simpler and reduce the need to pass null everywhere * address comments and cleanup

## Dependency updates - `@elastic/eui` - v113.0.0 ⏩ v113.1.0 - `@elastic/eui-theme-borealis` - v6.0.0 ⏩ v6.1.0 --- ## Changes | Commit | Change | Teams (Code Owner) | | :--- | :--- | :--- | | 18ef117 | Update snapshots, new Assistance color tokens ([#9383](elastic/eui#9383)) | @elastic/kibana-security | | 3fad87f | Update snapshots, new `aria-expanded` attribute added to button triggers of `EuiPopover` ([#9381](elastic/eui#9381)) | @elastic/appex-sharedux, @elastic/kibana-visualizations, @elastic/kibana-data-discovery | | cc9e096 | Update snapshots, remove `aria-controls` now handled by `EuiPopover` directly ([#9381](elastic/eui#9381)) | @elastic/kibana-security | ## Package updates ### `@elastic/eui` [v113.1.0](https://github.com/elastic/eui/releases/tag/v113.1.0) - Added `data-test-subj` attributes to `EuiFlyoutMenu` elements: back button, history dropdown, and history items. ([#9400](elastic/eui#9400)) - Added new assistance tokens: ([#9383](elastic/eui#9383)) - `euiTheme.colors.backgroundFilledAssistance` - `euiTheme.colors.backgroundLightAssistance` - `euiTheme.colors.backgroundBaseAssistance` - `euiTheme.components.buttons.backgroundAssistanceHover`, - `euiTheme.components.buttons.backgroundFilledAssistanceHover` - `euiTheme.colors.backgroundBaseInteractiveHoverAssistance` - `euiTheme.colors.borderStrongAssistance` - `euiTheme.colors.borderBaseAssistance` - `euiTheme.colors.textAssistance` - `euiTheme.colors.vis.euiColorVisAssistance` - `euiTheme.colors.severity.assistance` - `euiTheme.colors.vis.euiColorVis10` - `euiTheme.colors.vis.euiColorVis11` - `euiTheme.colors.vis.euiColorVisText10` - `euiTheme.colors.vis.euiColorVisText11` - Updated purple color palette shades 30-60 to slightly lighter values ([#9383](elastic/eui#9383)) **Accessibility** - Adds `aria-expanded` and `aria-controls` to the `EuiPopover` trigger button to improve screen reader context ([#9381](elastic/eui#9381)) ### `@elastic/eui-theme-borealis` v6.1.0 - Added new assistance tokens: ([#9383](elastic/eui#9383)) - `euiTheme.colors.backgroundFilledAssistance` - `euiTheme.colors.backgroundLightAssistance` - `euiTheme.colors.backgroundBaseAssistance` - `euiTheme.components.buttons.backgroundAssistanceHover`, - `euiTheme.components.buttons.backgroundFilledAssistanceHover` - `euiTheme.colors.backgroundBaseInteractiveHoverAssistance` - `euiTheme.colors.borderStrongAssistance` - `euiTheme.colors.borderBaseAssistance` - `euiTheme.colors.textAssistance` - `euiTheme.colors.vis.euiColorVisAssistance` - `euiTheme.colors.severity.assistance` - `euiTheme.colors.vis.euiColorVis10` - `euiTheme.colors.vis.euiColorVis11` - `euiTheme.colors.vis.euiColorVisText10` - `euiTheme.colors.vis.euiColorVisText11` - Updated purple color palette shades 30-60 to slightly lighter values ([#9383](elastic/eui#9383))

## Dependency updates - `@elastic/eui` - v113.0.0 ⏩ v113.1.0 - `@elastic/eui-theme-borealis` - v6.0.0 ⏩ v6.1.0 --- ## Changes | Commit | Change | Teams (Code Owner) | | :--- | :--- | :--- | | 18ef117 | Update snapshots, new Assistance color tokens ([elastic#9383](elastic/eui#9383)) | @elastic/kibana-security | | 3fad87f | Update snapshots, new `aria-expanded` attribute added to button triggers of `EuiPopover` ([elastic#9381](elastic/eui#9381)) | @elastic/appex-sharedux, @elastic/kibana-visualizations, @elastic/kibana-data-discovery | | cc9e096 | Update snapshots, remove `aria-controls` now handled by `EuiPopover` directly ([elastic#9381](elastic/eui#9381)) | @elastic/kibana-security | ## Package updates ### `@elastic/eui` [v113.1.0](https://github.com/elastic/eui/releases/tag/v113.1.0) - Added `data-test-subj` attributes to `EuiFlyoutMenu` elements: back button, history dropdown, and history items. ([elastic#9400](elastic/eui#9400)) - Added new assistance tokens: ([elastic#9383](elastic/eui#9383)) - `euiTheme.colors.backgroundFilledAssistance` - `euiTheme.colors.backgroundLightAssistance` - `euiTheme.colors.backgroundBaseAssistance` - `euiTheme.components.buttons.backgroundAssistanceHover`, - `euiTheme.components.buttons.backgroundFilledAssistanceHover` - `euiTheme.colors.backgroundBaseInteractiveHoverAssistance` - `euiTheme.colors.borderStrongAssistance` - `euiTheme.colors.borderBaseAssistance` - `euiTheme.colors.textAssistance` - `euiTheme.colors.vis.euiColorVisAssistance` - `euiTheme.colors.severity.assistance` - `euiTheme.colors.vis.euiColorVis10` - `euiTheme.colors.vis.euiColorVis11` - `euiTheme.colors.vis.euiColorVisText10` - `euiTheme.colors.vis.euiColorVisText11` - Updated purple color palette shades 30-60 to slightly lighter values ([elastic#9383](elastic/eui#9383)) **Accessibility** - Adds `aria-expanded` and `aria-controls` to the `EuiPopover` trigger button to improve screen reader context ([elastic#9381](elastic/eui#9381)) ### `@elastic/eui-theme-borealis` v6.1.0 - Added new assistance tokens: ([elastic#9383](elastic/eui#9383)) - `euiTheme.colors.backgroundFilledAssistance` - `euiTheme.colors.backgroundLightAssistance` - `euiTheme.colors.backgroundBaseAssistance` - `euiTheme.components.buttons.backgroundAssistanceHover`, - `euiTheme.components.buttons.backgroundFilledAssistanceHover` - `euiTheme.colors.backgroundBaseInteractiveHoverAssistance` - `euiTheme.colors.borderStrongAssistance` - `euiTheme.colors.borderBaseAssistance` - `euiTheme.colors.textAssistance` - `euiTheme.colors.vis.euiColorVisAssistance` - `euiTheme.colors.severity.assistance` - `euiTheme.colors.vis.euiColorVis10` - `euiTheme.colors.vis.euiColorVis11` - `euiTheme.colors.vis.euiColorVisText10` - `euiTheme.colors.vis.euiColorVisText11` - Updated purple color palette shades 30-60 to slightly lighter values ([elastic#9383](elastic/eui#9383))

tbragin added Feature:Dashboard Dashboard related features :Sharing labels Dec 16, 2016

stacey-gammon force-pushed the streamline-add-vis-dashboard branch 4 times, most recently from c50294f to a781c31 Compare December 30, 2016 19:57

stacey-gammon added the review label Dec 30, 2016

stacey-gammon changed the title ~~Streamline add vis dashboard (WIP/POC)~~ Streamline adding a visualization to a dashboard Dec 30, 2016

stacey-gammon force-pushed the streamline-add-vis-dashboard branch from a781c31 to 530346c Compare December 30, 2016 20:07

kobelb self-requested a review January 2, 2017 14:59

kobelb self-assigned this Jan 2, 2017

stacey-gammon assigned cjcenizal Jan 2, 2017

kobelb reviewed Jan 2, 2017

View reviewed changes

Stacey Gammon added 2 commits January 2, 2017 13:53

rebase with master

df8142d

Fix bug with param sticking around in the url causing multiple vis to…

4ff6030

… add again and again Move the link into the saved object finder and get rid of the management link fix crash in tests lint fixes

stacey-gammon force-pushed the streamline-add-vis-dashboard branch 2 times, most recently from b8dd23c to e2134ac Compare January 2, 2017 19:36

code review comments

0dd98e7

make the function simpler and reduce the need to pass null everywhere

stacey-gammon force-pushed the streamline-add-vis-dashboard branch from e2134ac to 0dd98e7 Compare January 2, 2017 19:37

kobelb approved these changes Jan 2, 2017

View reviewed changes

cjcenizal suggested changes Jan 2, 2017

View reviewed changes

address comments and cleanup

d18f14c

cjcenizal approved these changes Jan 3, 2017

View reviewed changes

stacey-gammon merged commit 344945d into elastic:master Jan 3, 2017

stacey-gammon mentioned this pull request Jan 4, 2017

[Dashboard] Streamlining add visualization #9554

Closed

elastic-jasper mentioned this pull request Jan 5, 2017

[5.x] Streamline adding a visualization to a dashboard #9749

Closed

stacey-gammon added the v5.3.0 label Jan 5, 2017

stacey-gammon mentioned this pull request Jan 5, 2017

[5.x] Streamline add visualization to dashboard Backport #9755

Closed

stacey-gammon mentioned this pull request Jan 5, 2017

[5.x] Streamline adding a visualization to a dashboard (#9400) #9756

Merged

stacey-gammon deleted the streamline-add-vis-dashboard branch February 7, 2017 14:37

cjcenizal mentioned this pull request Apr 4, 2017

Remove useLocalManagement attribute from SavedObjectFinder. #11028

Closed

Conversation

stacey-gammon commented Dec 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kobelb left a comment

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!

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!

kobelb left a comment

Choose a reason for hiding this comment

Uh oh!

cjcenizal left a comment

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!

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!

cjcenizal Jan 2, 2017 • 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!

stacey-gammon commented Jan 3, 2017

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

kobelb commented Jan 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

stacey-gammon commented Dec 7, 2016 •

edited

Loading

cjcenizal Jan 2, 2017 •

edited

Loading