Nested input controls #16407

nreese · 2018-01-30T17:34:47Z

This PR adds the ability to nest dropdown controls. A nested control is only enabled when all ancestor controls have values. The selectable options of a nested control are filtered by the values of all ancestor controls.

elasticmachine · 2018-01-30T18:22:50Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2018-01-30T19:09:25Z

@gchaps I need some help with better naming for the field parent and the help text. Thanks

gchaps · 2018-01-30T20:35:33Z

Nathan and I looked at different wording for parent (one being nest) and feel that parent/child terminology works best in this case. Our suggestion is to modify the wording to "Parent control." Here is the recommendation:

Parent control

Options are based on the value of parent control. Disabled if parent is not set.

elasticmachine · 2018-01-30T23:40:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-01-31T16:24:56Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-01-31T17:46:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb · 2018-02-02T19:35:32Z

src/core_plugins/input_control_vis/public/control/control.js

+    this.disable('Control has not been initialized');
+  }
+
+  async fetch() {


We're throwing an Error here because we require the subclass to define fetch, can we reflect that in the error message?

kobelb · 2018-02-02T20:00:55Z

src/core_plugins/input_control_vis/public/control/control.js

+   *
+   * @param ancestors {array of Controls}
+   */
+  setAncestors(ancestors) {


The RangeControl also extends from Control, but it doesn't do anything with ancestors, why are we adding all of these here?

I wanted to make the ancestor logic as generic as possible so I put it into the Control object. Maybe Range controls will be nestable in a future release.

kobelb · 2018-02-02T20:30:23Z

src/core_plugins/input_control_vis/public/lineage/parent_candidates.js

+export function getParentCandidates(controlParamsList, controlId, lineageMap) {
+  return controlParamsList.filter((controlParams) => {
+    // Ignore controls that do not have index pattern and field set
+    if (!controlParams.indexPattern || !controlParams.fieldName) {


Is this ignoring controlParams that haven't been "completed"? We have this same check elsewhere, can we generalize it?

kobelb · 2018-02-02T20:33:40Z

src/core_plugins/input_control_vis/public/lineage/lineage_map.js

+    const lineage = [rootControlParams.id];
+    const getLineage = (controlParams) => {
+      if (_.has(controlParams, 'parent') && controlParams.parent !== '' && !lineage.includes(controlParams.parent)) {
+        lineage.push(controlParams.parent);


If we stored the actual reference to the parent, could we skip the getControl in the VisController?

No. There are two sets of state.

control parameters - these are stored on the visualization saved object

control object - this just lives in heap and contains the current display state

getLineageMap only deals with control parameters

kobelb · 2018-02-02T20:40:38Z

src/core_plugins/input_control_vis/public/control/control.js

+    }, false);
+  }
+
+  getAncestorSignature() {


Why are we using string concatenation to determine object equality as opposed to using _.isEqual?

I needed some way to store a hash of the last query. Saving the values seemed like an easy way to create a hash that is easy to read and see what has changed. Also, the control objects contain lots of other state that I don't care about. Comparing objects would detect other changes that are not telling me that only the value of the control has changed.

Couldn't we just skip the .join then and compare the array of ancestor values?

kobelb · 2018-02-02T20:53:26Z

src/core_plugins/input_control_vis/public/control/control.js

+  }
+
+  getAncestorSearchSource() {
+    let previousAncestorSearchSource;


Why are we making all of the search sources inherit from their parent/grandparents/etc.? Don't we just want to be using the parent's value as the filter? That doesn't seem like it'd require walking up the parent hierarchy more than your direct parent.

You need the entire ancestry chain to ensure nested controls only contain options for the entire chain of selected values.

For example control1 = state, control2 = city, control3 = street name. There is a Rome Georgia and a Rome Ohio. If you only filtered by the parent control then your street name control would get results for all cities with the name Rome. But if you filter by the entire ancestry chain then the street name control would only get results where the State is Georgia and the City is Rome.

Gotcha, that makes sense why we'd need to add the filters for all ancestors, but I'm still not following why we need the search source inheritance.

kobelb · 2018-02-02T20:56:00Z

src/core_plugins/input_control_vis/public/control/list_control_factory.js

+      if (this.hasUnsetAncestor()) {
+        this.disable(`Disabled until '${this.ancestors[0].label}' is set.`);
+        // Remove any existing filters for control.
+        this.filterManager.findFilters().forEach((existingFilter) => {


If you add a parent and child filters, pin both of them and then remove the parent filter via the input control, it's removing the pinned filter.

That is the intended behavior. The filter pill is owned by the Control Panel. When the parent is unset, then the child state can only be unknown since there is no way of getting a valid options list

Even when I'm manually pinning the pills? It seems unnatural for me to pin a filter, and then have something else remove it. Perhaps this gif will better explain what I'm seeing:

It's possible that this behavior was present before this PR with non-parent/child pills, but I only noticed it now.

kobelb · 2018-02-02T20:56:30Z

src/core_plugins/input_control_vis/public/control/list_control_factory.js

+      }
+
+      const ancestorSignature = this.getAncestorSignature();
+      if (ancestorSignature === this.lastAncestorSignature) {


Even when it's using the timefilter value and time has changed?

elasticmachine · 2018-02-02T23:14:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

nreese · 2018-02-03T00:13:27Z

jenkins, test this

elasticmachine · 2018-02-03T00:59:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-05T19:08:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

kobelb

Centralizing the searchSource logic made this much easier for me to follow.

The only other outstanding questions I had were in regard to the way that we're short-circuiting when the ancestor's signature hasn't changed.

kobelb

LGTM

elasticmachine · 2018-02-06T17:19:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-02-08T14:15:56Z

src/core_plugins/input_control_vis/public/control/create_search_source.js

-    });
-  }
+  searchSource.filter(() => {
+    const activeFilters = [].concat(filters);


I think we try to use ES2016 syntax for that kind of things: const activeFilters = [...filters];

…rent' field copy

…cestors have changed

elasticmachine · 2018-02-08T16:35:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* add input to select parent control * move lineage logic into seperate file for easier testing * move parent candidate logic into seperate file * decouple control factory and control initialization * disable control if parent not set * inherite search source from ancestors to have them filter values * call fetch when query filter is updated for filter value changes * delete filters when control has ancestor without value * avoid fetching option list when ancestors have not changed * add functional tests for nested controls * remove unneeded file * fix jest tests, add another test case to functional tests, update 'Parent' field copy * add more jest test coverage * fix useTimeFilter functionallity * add better error message when fetch is not defined by subclass * pass filters array instead of search source hierarchy * only modify queryFilter when filters are submitted * compare value array instead of concatinated string to determine if ancestors have changed * use ES2016 syntax for array concatination

nreese added the Feature:Input Control Input controls visualization label Jan 30, 2018

nreese force-pushed the nested_controls branch from faf78f1 to 095fec6 Compare January 30, 2018 17:40

nreese added the release_note:enhancement label Jan 30, 2018

nreese changed the title ~~[WIP] Nested input controls~~ Nested input controls Jan 31, 2018

nreese requested review from kobelb and timroes January 31, 2018 17:47

kobelb reviewed Feb 2, 2018

View reviewed changes

kobelb reviewed Feb 6, 2018

View reviewed changes

kobelb approved these changes Feb 6, 2018

View reviewed changes

timroes approved these changes Feb 8, 2018

View reviewed changes

nreese added 9 commits February 8, 2018 08:46

add input to select parent control

911502f

move lineage logic into seperate file for easier testing

a608c6e

move parent candidate logic into seperate file

39470c0

decouple control factory and control initialization

f8fe26e

disable control if parent not set

0f20c52

inherite search source from ancestors to have them filter values

526a429

call fetch when query filter is updated for filter value changes

20299eb

delete filters when control has ancestor without value

d5adff8

avoid fetching option list when ancestors have not changed

826c8bf

nreese added 9 commits February 8, 2018 08:46

add functional tests for nested controls

c8708f9

remove unneeded file

d42eb31

fix jest tests, add another test case to functional tests, update 'Pa…

4ff0677

…rent' field copy

add more jest test coverage

4a28660

fix useTimeFilter functionallity

2af6265

add better error message when fetch is not defined by subclass

a85ed8f

pass filters array instead of search source hierarchy

5877795

only modify queryFilter when filters are submitted

1bd125e

compare value array instead of concatinated string to determine if an…

1ffd53b

…cestors have changed

nreese force-pushed the nested_controls branch from 3404b62 to 1ffd53b Compare February 8, 2018 15:46

use ES2016 syntax for array concatination

d3f07ba

nreese merged commit 8664b55 into elastic:master Feb 8, 2018

nreese mentioned this pull request Feb 8, 2018

[6.x] Nested input controls (#16407) #16612

Merged

nreese deleted the nested_controls branch February 13, 2018 19:30

nreese mentioned this pull request Feb 22, 2018

Add "use time filter" option to input controls #15852

Merged

stacey-gammon mentioned this pull request Feb 27, 2018

Controls should update based on value selected. #16923

Closed

nreese mentioned this pull request Apr 9, 2018

Input Controls visualization should not ignore dashboard filter #16123

Closed

aphelionz mentioned this pull request May 7, 2018

Accessibility: Kibana 6.3 Meta Issue #18866

Closed

48 tasks

kobelb added v7.0.0 v6.3.0 labels May 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested input controls #16407

Nested input controls #16407

nreese commented Jan 30, 2018 •

edited

Loading

elasticmachine commented Jan 30, 2018

nreese commented Jan 30, 2018

gchaps commented Jan 30, 2018

elasticmachine commented Jan 30, 2018

elasticmachine commented Jan 31, 2018

elasticmachine commented Jan 31, 2018

kobelb Feb 2, 2018

kobelb Feb 2, 2018

nreese Feb 2, 2018

kobelb Feb 2, 2018

kobelb Feb 2, 2018

nreese Feb 2, 2018

kobelb Feb 2, 2018

nreese Feb 2, 2018 •

edited

Loading

kobelb Feb 5, 2018

kobelb Feb 2, 2018

nreese Feb 2, 2018

kobelb Feb 5, 2018

kobelb Feb 2, 2018

nreese Feb 2, 2018

kobelb Feb 5, 2018

kobelb Feb 2, 2018

elasticmachine commented Feb 2, 2018

nreese commented Feb 3, 2018

elasticmachine commented Feb 3, 2018

elasticmachine commented Feb 5, 2018

kobelb left a comment

kobelb left a comment

elasticmachine commented Feb 6, 2018

timroes Feb 8, 2018

elasticmachine commented Feb 8, 2018

Nested input controls #16407

Nested input controls #16407

Conversation

nreese commented Jan 30, 2018 • edited Loading

elasticmachine commented Jan 30, 2018

💔 Build Failed

nreese commented Jan 30, 2018

gchaps commented Jan 30, 2018

elasticmachine commented Jan 30, 2018

💔 Build Failed

elasticmachine commented Jan 31, 2018

💔 Build Failed

elasticmachine commented Jan 31, 2018

💚 Build Succeeded

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese Feb 2, 2018 • 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

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

Choose a reason for hiding this comment

elasticmachine commented Feb 2, 2018

💔 Build Failed

nreese commented Feb 3, 2018

elasticmachine commented Feb 3, 2018

💚 Build Succeeded

elasticmachine commented Feb 5, 2018

💚 Build Succeeded

kobelb left a comment

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

elasticmachine commented Feb 6, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

elasticmachine commented Feb 8, 2018

💚 Build Succeeded

nreese commented Jan 30, 2018 •

edited

Loading

nreese Feb 2, 2018 •

edited

Loading