Add comments and inline docs for visualization saving and editing process. #8208

cjcenizal · 2016-09-09T20:25:57Z

Supersedes #7968

The goal is to clarify where URL state is coming from, when working with visualizations. Some of the classes touched upon are: SavedVis, PersistedState, AppState, and base classes.

Explored files:

src/core_plugins/kibana/public/visualize/editor/editor.js
src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js
src/ui/public/courier/data_source/_doc_send_to_es.js
src/ui/public/courier/data_source/doc_source.js
src/ui/public/courier/saved_object/saved_object.js
src/ui/public/es.js
src/ui/public/events.js
src/ui/public/persisted_state/persisted_state.js
src/ui/public/state_management/app_state.js
src/ui/public/state_management/state.js
src/ui/public/vis/agg_config.js
src/ui/public/vis/agg_configs.js
src/ui/public/vis/vis.js

epixa · 2016-10-08T15:48:14Z

@cjcenizal What's the status of this?

cjcenizal · 2016-10-10T22:45:20Z

@epixa I'll get this merged this week.

tylersmalley

Just found one nitpick, but other than that it was a very informative PR.

tylersmalley · 2016-10-10T23:46:14Z

src/ui/public/es.js

@@ -2,6 +2,12 @@ import 'elasticsearch-browser';
 import _ from 'lodash';
 import uiModules from 'ui/modules';

+/**
+ * @name es
+ * @description This is the result of calling esFactory. Where is esFactory


To keep this PR consistent, can we place a line break between name and description?

thomasneirynck · 2016-10-10T23:59:34Z

LGTM

spalger

Some of the descriptions here feel more like observations, and not descriptions of the functionality they label. I'm all for sharing your observations but I would rather only put full-formed descriptions into the code.

spalger · 2016-10-11T00:19:53Z

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

    requesting.call(vis);
+
+    // The benefit of calling `editableVis.requesting()` isn't clear.


Is this not explained above?

When a vis is marked as being requested, the bounds of that vis are updated and new data is fetched using the new bounds.

You actually made this comment on the original PR: #7968 (comment)

spalger · 2016-10-11T00:20:26Z

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

  const savedVis = $route.current.locals.savedVis;

+  // Instance of vis.js.


I think that in order for this to be useful the comment should point to the actual file, not just vis.js

spalger · 2016-10-11T00:21:17Z

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

@@ -115,6 +127,8 @@ function VisEditor($scope, $route, timefilter, AppState, $location, kbnUrl, $tim
    docTitle.change(savedVis.title);
  }

+  // Extract visualization state with filtered aggs.
+  // Consists of: aggs, params, listeners, title, and type.


I think this should probably point the reader to where they can find the current state (like the URL) rather than list out it's properties. Otherwise the list will just get out of date and not benefit anyone

Sounds good but not sure what you mean re point out where the reader can find current state. Can you give me an example of the comment you have in mind?

spalger · 2016-10-11T00:24:06Z

src/ui/public/courier/data_source/_doc_send_to_es.js

+/**
+ * @name _doc_send_to_es
+ *
+ * @description Depends upon the es object to make ES requests, and also


I don't think this comment is useful

It's not the most insightful comment, but it does give some idea of the relationship with the other actors in our codebase, so I'd like to leave it unless we can come up with a more meaningful description. :)

spalger · 2016-10-11T00:27:45Z

src/ui/public/courier/data_source/doc_source.js

+/**
+ * @name DocSource
+ *
+ * @description This class is tightly coupled with _doc_send_to_es. Its primary


The relationship between this class and _doc_send_to_es doesn't really describe the class at all

I think you'll need to illuminate me...

spalger · 2016-10-11T00:39:07Z

src/ui/public/es.js

+/**
+ * @name es
+ *
+ * @description This is the result of calling esFactory. Where is esFactory


esFactory is exposed by the elasticsearch.angular.js client

spalger · 2016-10-11T00:40:06Z

src/ui/public/vis/vis.js

@@ -24,7 +34,6 @@ export default function VisFactory(Notifier, Private) {

    this.indexPattern = indexPattern;

-    // http://aphyr.com/data/posts/317/state.gif


party pooper

spalger · 2016-10-11T00:44:24Z

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

-    if (!angular.equals($state.vis, savedVisState)) {
+    // This is used to generate a 'uiState' PersistedState instance, and
+    // implictly add 'change' and 'fetch_with_changes' event handlers.
+    const appState = new AppState(stateDefaults);


I don't see how the change or fetch_with_changes listeners are of any importance here. All that maters is that the properties of the appState are synced with the url, right? In order to save changes call appState.save() any changed from the url will be reflected in the properties of the appState

spalger · 2016-10-11T00:46:29Z

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

@@ -258,6 +285,9 @@ function VisEditor($scope, $route, timefilter, AppState, $location, kbnUrl, $tim
    kbnUrl.change('/visualize', {});
  };

+  /**
+   * Called when the user clicks "Save" button.


This is probably unnecessary

I would agree if the name of the method was something like onClickSave to indicate that this is part of the interface exposed to the template. But since the role of this method is a little unclear from the name, I'd like to leave this comment until we refactor and clarify naming.

spalger · 2016-10-11T00:47:02Z

src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js

+ *
+ * @extends SavedObject.
+ *
+ * @description It's a type of SavedObject, but specific to visualizations.


Not seeing the value in this description

cjcenizal · 2016-10-11T18:41:26Z

jenkins, test this

…cess. The goal is to clarify where URL state is coming from, when working with visualizations. Some of the classes touched upon are: SavedVis, PersistedState, AppState, and base classes. Explored files: - src/core_plugins/kibana/public/visualize/editor/editor.js - src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js - src/ui/public/courier/data_source/_doc_send_to_es.js - src/ui/public/courier/data_source/doc_source.js - src/ui/public/courier/saved_object/saved_object.js - src/ui/public/es.js - src/ui/public/events.js - src/ui/public/persisted_state/persisted_state.js - src/ui/public/state_management/app_state.js - src/ui/public/state_management/state.js - src/ui/public/vis/agg_config.js - src/ui/public/vis/agg_configs.js - src/ui/public/vis/vis.js

epixa · 2016-10-11T19:39:27Z

@cjcenizal To help in our ongoing quest to make backports easier, please backport this to 5.x and 5.0

--------- **Commit 1:** Add comments and inline docs for visualization saving and editing process. The goal is to clarify where URL state is coming from, when working with visualizations. Some of the classes touched upon are: SavedVis, PersistedState, AppState, and base classes. Explored files: - src/core_plugins/kibana/public/visualize/editor/editor.js - src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js - src/ui/public/courier/data_source/_doc_send_to_es.js - src/ui/public/courier/data_source/doc_source.js - src/ui/public/courier/saved_object/saved_object.js - src/ui/public/es.js - src/ui/public/events.js - src/ui/public/persisted_state/persisted_state.js - src/ui/public/state_management/app_state.js - src/ui/public/state_management/state.js - src/ui/public/vis/agg_config.js - src/ui/public/vis/agg_configs.js - src/ui/public/vis/vis.js * Original sha: 34e3f49 * Authored by CJ Cenizal <[email protected]> on 2016-08-10T00:10:48Z

[backport] PR #8208 to 5.0

[backport] PR #8208 to 5.x - Add comments and inline docs for visualization saving and editing process.

cjcenizal · 2016-10-11T19:43:19Z

Backported to 5.0 and 5.x.

--------- **Commit 1:** Add comments and inline docs for visualization saving and editing process. The goal is to clarify where URL state is coming from, when working with visualizations. Some of the classes touched upon are: SavedVis, PersistedState, AppState, and base classes. Explored files: - src/core_plugins/kibana/public/visualize/editor/editor.js - src/core_plugins/kibana/public/visualize/saved_visualizations/_saved_vis.js - src/ui/public/courier/data_source/_doc_send_to_es.js - src/ui/public/courier/data_source/doc_source.js - src/ui/public/courier/saved_object/saved_object.js - src/ui/public/es.js - src/ui/public/events.js - src/ui/public/persisted_state/persisted_state.js - src/ui/public/state_management/app_state.js - src/ui/public/state_management/state.js - src/ui/public/vis/agg_config.js - src/ui/public/vis/agg_configs.js - src/ui/public/vis/vis.js * Original sha: 3cf76b6004f98d694cb0d31831035e806f71357c [formerly 34e3f49] * Authored by CJ Cenizal <[email protected]> on 2016-08-10T00:10:48Z Former-commit-id: 433c265

[backport] PR elastic#8208 to 5.x - Add comments and inline docs for visualization saving and editing process. Former-commit-id: 8752eea

cjcenizal added review v6.0.0 labels Oct 10, 2016

cjcenizal assigned spalger Oct 10, 2016

cjcenizal force-pushed the improvement/document-url-state-files branch 2 times, most recently from b7285f5 to fcd9615 Compare October 10, 2016 23:47

tylersmalley approved these changes Oct 10, 2016

View reviewed changes

cjcenizal force-pushed the improvement/document-url-state-files branch from fcd9615 to b94253a Compare October 10, 2016 23:50

thomasneirynck self-assigned this Oct 10, 2016

spalger suggested changes Oct 11, 2016

View reviewed changes

cjcenizal force-pushed the improvement/document-url-state-files branch 2 times, most recently from f191358 to bd21af2 Compare October 11, 2016 18:04

spalger approved these changes Oct 11, 2016

View reviewed changes

cjcenizal force-pushed the improvement/document-url-state-files branch from bd21af2 to 00f532e Compare October 11, 2016 18:21

cjcenizal force-pushed the improvement/document-url-state-files branch from 00f532e to 34e3f49 Compare October 11, 2016 19:10

cjcenizal merged commit 4c45c26 into elastic:master Oct 11, 2016

cjcenizal deleted the improvement/document-url-state-files branch October 11, 2016 19:11

cjcenizal removed the review label Oct 11, 2016

elastic-jasper mentioned this pull request Oct 11, 2016

[backport] PR #8208 to 5.0 - Add comments and inline docs for visualization saving and editing process. #8624

Merged

elastic-jasper mentioned this pull request Oct 11, 2016

[backport] PR #8208 to 5.x - Add comments and inline docs for visualization saving and editing process. #8625

Merged

cjcenizal added a commit that referenced this pull request Oct 11, 2016

Merge pull request #8624 from elastic/jasper/backport/8208/5.0

772889f

[backport] PR #8208 to 5.0

cjcenizal added a commit that referenced this pull request Oct 11, 2016

Merge pull request #8625 from elastic/jasper/backport/8208/5.x

8752eea

[backport] PR #8208 to 5.x - Add comments and inline docs for visualization saving and editing process.

jimgoodwin added the v6.0.0-rc1 label Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add comments and inline docs for visualization saving and editing process. #8208

Add comments and inline docs for visualization saving and editing process. #8208

cjcenizal commented Sep 9, 2016 •

edited

Loading

epixa commented Oct 8, 2016

cjcenizal commented Oct 10, 2016

tylersmalley left a comment

tylersmalley Oct 10, 2016

thomasneirynck commented Oct 10, 2016

spalger left a comment

spalger Oct 11, 2016 •

edited

Loading

cjcenizal Oct 11, 2016

spalger Oct 11, 2016

spalger Oct 11, 2016 •

edited

Loading

cjcenizal Oct 11, 2016

spalger Oct 11, 2016

cjcenizal Oct 11, 2016

spalger Oct 11, 2016 •

edited

Loading

cjcenizal Oct 11, 2016

spalger Oct 11, 2016

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

cjcenizal Oct 11, 2016

spalger Oct 11, 2016

cjcenizal Oct 11, 2016

cjcenizal commented Oct 11, 2016

epixa commented Oct 11, 2016

cjcenizal commented Oct 11, 2016

		requesting.call(vis);

		// The benefit of calling `editableVis.requesting()` isn't clear.

		const savedVis = $route.current.locals.savedVis;

		// Instance of vis.js.

		@@ -24,7 +34,6 @@ export default function VisFactory(Notifier, Private) {

		this.indexPattern = indexPattern;

		// http://aphyr.com/data/posts/317/state.gif

Add comments and inline docs for visualization saving and editing process. #8208

Add comments and inline docs for visualization saving and editing process. #8208

Conversation

cjcenizal commented Sep 9, 2016 • edited Loading

epixa commented Oct 8, 2016

cjcenizal commented Oct 10, 2016

tylersmalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasneirynck commented Oct 10, 2016

spalger left a comment

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

spalger Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Oct 11, 2016

epixa commented Oct 11, 2016

cjcenizal commented Oct 11, 2016

cjcenizal commented Sep 9, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading

spalger Oct 11, 2016 •

edited

Loading