Removes the "Index contains time-based events" checkbox #11409

ycombinator · 2017-04-25T15:09:00Z

This PR addresses the first part of #10444.

Currently we show a checkbox labeled "Index contains time-based events". The user has to check this box, and is then shown a select list with time fields.

This PR simplifies this UX by:

Taking away the checkbox and always showing the select list
For valid index patterns, showing the first item in the select list as "No Time Filter field". For empty or non-existing index patterns, the select list is empty
For index patterns with no time fields, disabling the select list
For index patterns with time fields, enabling the select list
Renaming the label of the select list to "Time filter field name" to better reflect where the selected time field is used
[Cleanup in related code] Renaming the label for the time field on the Edit Index Pattern page to "Time filter field name" for consistency

When no index pattern is specified

When a non-existing index name/pattern is specified

When an index containing no time fields is specified

When an index containing time fields is specified

ycombinator · 2017-04-25T15:09:39Z

/cc @skearns64

epixa · 2017-04-25T15:10:49Z

I assume you wanted the v6.0.0 label here :)

epixa · 2017-04-25T15:19:22Z

Two suggestions:

Can you move the "Unable to fetch mappings" error message to immediately below the index pattern field? This way the error appears at the place that caused it/where it can be fixed. You may need to play around with a disabled submit button instead of hiding the submit button entirely, but honestly this is really how the form should work anyway. Not showing a submit button until everything is kosher is kind of awkward. I don't want to churn too much on this, so if this seems to blow open the issue, we can address it in a follow up.

Rather than "No Time Filter Field", which doesn't do a great job describing why that may be the case or what the effect may be, how about two different possibilities:

For indices without time fields at all: Not time-based: Matching indices do not have date fields

For indices with time fields but without one selected: Not time-based: No date field selected as the primary time field

Thoughts?

ycombinator · 2017-04-25T15:28:23Z

Can you move the "Unable to fetch mappings" error message to immediately below the index pattern field? This way the error appears at the place that caused it/where it can be fixed. You may need to play around with a disabled submit button instead of hiding the submit button entirely, but honestly this is really how the form should work anyway. Not showing a submit button until everything is kosher is kind of awkward. I don't want to churn too much on this, so if this seems to blow open the issue, we can address it in a follow up.

Yeah, I agree with this change but I'd like to address it in a follow up PR. I've created an issue so we don't forget about it: #11410.

Rather than "No Time Filter Field", which doesn't do a great job describing why that may be the case or what the effect may be, how about two different possibilities:

For indices without time fields at all: Not time-based: Matching indices do not have date fields

For indices with time fields but without one selected: Not time-based: No date field selected as the primary time field

Yeah, ++ to making the messages more helpful to the user. Changing.

cjcenizal · 2017-04-25T16:01:30Z

When an index containing no time fields is specified

In this situation, can we replace the select with some text stating, "The indices which match this index pattern don't contain any time fields."

When an index containing time fields is specified

Can we change "No time filter field" to "None selected"? The former sounds like there simply are no time filter fields available, and I think the latter clarifies that the user hasn't picked one yet.

Edit: This is basically echoing Court's suggestions, but with my own spin on them.

ycombinator · 2017-04-25T16:02:45Z

@cjcenizal Did you see Court's comment earlier? It addresses the same issues you brought up. I'm putting up a new commit with some better verbiage shortly.

cjcenizal · 2017-04-25T16:04:05Z

@ycombinator Yeah I saw his comment afterwards and edited my comment. We have the same things in mind, but I think the language I suggested is a little more human-readable.

ycombinator · 2017-04-25T16:05:17Z

Okay, changing to the language suggested by CJ.

cjcenizal · 2017-04-25T16:21:57Z

Winning!

ycombinator · 2017-04-25T16:54:46Z

I updated the verbiage per @cjcenizal's suggestions but I kept the select lists always in place per the spirit of the conversation here (or my understanding of it anyway): #10444 (comment).

Updated screenshots for all the cases below.

When no index pattern is specified

When a non-existing index name/pattern is specified

When an index containing no time fields is specified

When an index containing time fields is specified

cjcenizal · 2017-04-25T17:44:04Z

@ycombinator What do you think of this? The reason I suggest these changes is because I think we need to explain what's going on to the user a bit more, and I don't want to overload the select component with a ton of text.

If you think this works, we can implement it by adding a paragraph below the select:

<p class="help-block" ng-if="!controller.doesIndexHaveDateFields">
  The indices which match this index pattern don&rsquo;t contain any time fields.
</p>

And updating the controller like this:

if (this.dateFields.length === 0) {
  this.doesIndexHaveDateFields = false;
  this.dateFields.unshift(TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX);
} else {
  this.doesIndexHaveDateFields = true;
  this.dateFields.unshift(TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED);
}

And making this change in the translation file:

  "KIBANA-NO_DATE_FIELD_SELECTED": "None selected",
  "KIBANA-NO_DATE_FIELDS_IN_INDEX": "None available",

cjcenizal

Looks awesome! Just had some UX and other small code suggestions.

cjcenizal · 2017-04-25T17:46:33Z

...ins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.html

            <label>
-              <span translate="KIBANA-TIME_FIELD_NAME"></span>
+              <span translate="KIBANA-TIME_FILTER_FIELD_NAME"></span>


Just curious: what does time filter mean in this context?

I'm using that per this suggestion: #10444 (comment).

I think "time picker" might be more appropriate but I'm not sure if users know what that is.

Hmmm... I guess "time filter" works. I like "time picker" because I know what that is, but I'm afraid that might couple these field too closely to a particular UI component.

cjcenizal · 2017-04-25T17:49:54Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+      name: $translate.instant('KIBANA-NO_DATE_FIELD_SELECTED')
+    },
+    NO_DATE_FIELDS_IN_INDEX: {
+      name: $translate.instant('KIBANA-NO_DATE_FIELDS_IN_INDEX')


Really minor suggestion: technically, we're referring to "indices", and not a single index, right? It all depends on how many indices match the index pattern. Maybe it's just me, but when I first started using Kibana, it took awhile for me to understand the difference between index pattern and index. I think renaming these to be plural will help reinforce the relationship, e.g. NO_DATE_FIELDS_IN_INDICES and KIBANA-NO_DATE_FIELDS_IN_INDICES.

cjcenizal · 2017-04-25T17:52:40Z

...ins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.html

-          <div class="form-group time-and-pattern">
-            <label>
-              <input
-                data-test-subj="createIndexPatternIsTimeBasedCheckBox"


We'll need to update the functional tests to remove references to this element.

cjcenizal · 2017-04-25T17:54:54Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

-      : undefined;
+    let timeFieldName;
+    if ((this.newIndexPattern.timeField !== TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED)
+      && (this.newIndexPattern.timeField !== TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX)) {


I think this could be reduced to:

![ TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELD_SELECTED, TIME_FILTER_FIELD_OPTIONS.NO_DATE_FIELDS_IN_INDEX ].includes(this.newIndexPattern.timeField)

Yeah, I considered something like that but I find it less readable. In particular, I found the .includes relationship to be inverted (in terms of readability).

I'd be fine with shortening this if we could express this in reverse, so something like (pseudocode):

!this.newIndexPattern.timeField.isOneOf([ ... ])

But I don't think there's any way to do that, either in JS itself or using a helper library.

So I'd prefer to leave it the way it-is for added readability.

If you strongly disagree, we can get a third person to break the tie.

It's all good, I don't feel very strongly about this.

ycombinator · 2017-04-25T19:16:17Z

@cjcenizal This is ready for review again, based on your feedback. I still need to implement the spinner though, so if you just want to wait till that's done, that's cool too.

cjcenizal

LGTM! Thanks for making those UI changes.

ycombinator · 2017-04-25T20:03:26Z

@cjcenizal On second thought, I'm going to tackle the spinner as a separate PR, since there's a neat little separate issue for it that specs it out well already: #10439.

rhoboat · 2017-04-25T20:25:57Z

@cjcenizal @ycombinator Just being a ceiling cat here, but in the gif that CJ posted, I couldn't tell that the text The indices which match this index pattern don't contain any time fields. appeared below as a message corresponding to the change in the input box. Maybe flashing a background color highlight that fades after a second would help draw attention?

w33ble

Couple small things, up to you if you want to make the changes. lgtm.

w33ble · 2017-04-26T20:53:36Z

...ins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.html

@@ -63,13 +53,16 @@
            </label>
            <select
              data-test-subj="createIndexPatternTimeFieldSelect"
-              ng-disabled="controller.fetchFieldsError"
+              ng-disabled="controller.fetchFieldsError || (controller.dateFields.length === 1)"


Might be clearer to use !controller.doesIndexHaveDateFields here.

Ah yes, that was introduced later and should definitely be used here. Thanks!

w33ble · 2017-04-26T20:54:40Z

...ugins/kibana/public/management/sections/indices/create_index_pattern/create_index_pattern.js

+    }
+
+    this.dateFields = results.dateFields || [];
+    this.doesIndexHaveDateFields = this.dateFields.length > 0;


this.indexHasDateFields fits the pattern better, and reads better when used for conditionals.

cjcenizal · 2017-04-26T21:46:51Z

@archanid Good point! I think it's better to address that as part of our new notifications designs. Would you mind chiming in with your thoughts and cross-linking on #8194 ?

ycombinator · 2017-04-26T23:19:37Z

Tests are failing but they are unrelated to the changes in this PR. Merging...

* Removing "Index contains time-based events" checkbox * Rename label * Adding "No Time Filter field" to time field select list * Using translation for i18n and consistency with label on create form * More useful verbiage * Updating copy * Adding help block below select for added clarity * Removing functional tests that used time-based events checkbox * s/INDEX/INDICES/ to be reflect reality * Fixing logic error * Updating functional test to match new behavior * Use controller property instead of inline expression * More readable variable name

ycombinator · 2017-04-26T23:26:50Z

Backported to:

5.x / 5.5: 7aae24b

In elastic#11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created.

* [indexPatterns/create] timeFields -> timeFieldOptions In #11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created. * [indexPatterns/create] populate `this.timeFieldOptions` * [indexPatterns/create] unify fetching and errors for timeFieldOptions * [indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions() * [indexPatterns/create] unify "No time fields available" messages * [indexPatterns/create] disable the timeField select when loading * [indexPatterns/create] only require a time field is there are options Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading * [indexPatterns/create] isolate side-effects * [indexPatterns/create] let refreshTimeFieldOptions() control model * [indexPatterns/create] attempt to make default field selection clearer * [indexPatterns/create] explain why we set timeFieldOption * [indexPatterns/create] make timeFieldName `undefined` when not set * [indexPatterns/create] Don't pass two args to .then() unless needed * 💅

* [indexPatterns/create] timeFields -> timeFieldOptions In #11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created. * [indexPatterns/create] populate `this.timeFieldOptions` * [indexPatterns/create] unify fetching and errors for timeFieldOptions * [indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions() * [indexPatterns/create] unify "No time fields available" messages * [indexPatterns/create] disable the timeField select when loading * [indexPatterns/create] only require a time field is there are options Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading * [indexPatterns/create] isolate side-effects * [indexPatterns/create] let refreshTimeFieldOptions() control model * [indexPatterns/create] attempt to make default field selection clearer * [indexPatterns/create] explain why we set timeFieldOption * [indexPatterns/create] make timeFieldName `undefined` when not set * [indexPatterns/create] Don't pass two args to .then() unless needed * 💅 (cherry picked from commit 801549c)

* [indexPatterns/create] timeFields -> timeFieldOptions In elastic#11409 we added two new possible options to the "Time Filter field name" drop down. These options display a message that "there are no time fields" or "I don't want to use the time filter". These new options were added to the `controller.timeFields` array, which is where the other options were, but they don't actually represent time fields. This change updates `controller.timeFields` to instead be a list of options, specifically created to populate the select box in the view. They are now stored at `controller.timeFieldOptions` and each option has a `display` property (which will be rendered in the view) and optionally a `fieldName` property. The selected option, `controller.formValues.timeFieldOption` is checked for a `fieldName` property when the Index Pattern is created. * [indexPatterns/create] populate `this.timeFieldOptions` * [indexPatterns/create] unify fetching and errors for timeFieldOptions * [indexPatterns/create] refreshFieldList() -> refreshTimeFieldOptions() * [indexPatterns/create] unify "No time fields available" messages * [indexPatterns/create] disable the timeField select when loading * [indexPatterns/create] only require a time field is there are options Doing this prevents the time field from rendering as invalid when there is no matching index pattern (which has it's own error display) and when the fields are loading * [indexPatterns/create] isolate side-effects * [indexPatterns/create] let refreshTimeFieldOptions() control model * [indexPatterns/create] attempt to make default field selection clearer * [indexPatterns/create] explain why we set timeFieldOption * [indexPatterns/create] make timeFieldName `undefined` when not set * [indexPatterns/create] Don't pass two args to .then() unless needed * 💅

ycombinator added 4 commits April 25, 2017 05:59

Removing "Index contains time-based events" checkbox

203c89c

Rename label

997adf7

Adding "No Time Filter field" to time field select list

4064c99

Using translation for i18n and consistency with label on create form

df0e18b

ycombinator added :Management review v4.6.0 v5.5.0 labels Apr 25, 2017

ycombinator assigned w33ble and cjcenizal Apr 25, 2017

ycombinator requested review from w33ble and cjcenizal April 25, 2017 15:09

epixa added v6.0.0 and removed v4.6.0 labels Apr 25, 2017

More useful verbiage

dba8583

cjcenizal suggested changes Apr 25, 2017

View reviewed changes

ycombinator added 3 commits April 25, 2017 11:53

Updating copy

442467a

Adding help block below select for added clarity

5348a55

Removing functional tests that used time-based events checkbox

d60eda0

ycombinator added 2 commits April 25, 2017 11:58

s/INDEX/INDICES/ to be reflect reality

244cb72

Fixing logic error

c3270a6

cjcenizal mentioned this pull request Apr 25, 2017

Convert Index Pattern Creation form UI to use UI Framework components. #11419

Merged

cjcenizal approved these changes Apr 25, 2017

View reviewed changes

Updating functional test to match new behavior

3688f12

w33ble approved these changes Apr 26, 2017

View reviewed changes

ycombinator added 2 commits April 26, 2017 14:58

Use controller property instead of inline expression

a1c86d7

More readable variable name

f701b58

ycombinator merged commit 02787e3 into elastic:master Apr 26, 2017

ycombinator deleted the mgmt/kibana/index-patterns/create/default-time-filter-field branch April 26, 2017 23:27

spalger mentioned this pull request May 25, 2017

[indexPatterns/create] refactor time field options #11996

Merged

jimgoodwin added the release_note:enhancement label Jun 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removes the "Index contains time-based events" checkbox #11409

Removes the "Index contains time-based events" checkbox #11409

ycombinator commented Apr 25, 2017 •

edited

Loading

ycombinator commented Apr 25, 2017

epixa commented Apr 25, 2017

epixa commented Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017 •

edited

Loading

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017 •

edited

Loading

cjcenizal left a comment

cjcenizal Apr 25, 2017

ycombinator Apr 25, 2017

cjcenizal Apr 25, 2017

cjcenizal Apr 25, 2017

cjcenizal Apr 25, 2017

cjcenizal Apr 25, 2017

ycombinator Apr 25, 2017

cjcenizal Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal left a comment

ycombinator commented Apr 25, 2017

rhoboat commented Apr 25, 2017

w33ble left a comment

w33ble Apr 26, 2017

ycombinator Apr 26, 2017

w33ble Apr 26, 2017

cjcenizal commented Apr 26, 2017

ycombinator commented Apr 26, 2017

ycombinator commented Apr 26, 2017

Removes the "Index contains time-based events" checkbox #11409

Removes the "Index contains time-based events" checkbox #11409

Conversation

ycombinator commented Apr 25, 2017 • edited Loading

When no index pattern is specified

When a non-existing index name/pattern is specified

When an index containing no time fields is specified

When an index containing time fields is specified

ycombinator commented Apr 25, 2017

epixa commented Apr 25, 2017

epixa commented Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017 • edited Loading

When an index containing no time fields is specified

When an index containing time fields is specified

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017

ycombinator commented Apr 25, 2017

cjcenizal commented Apr 25, 2017

ycombinator commented Apr 25, 2017

When no index pattern is specified

When a non-existing index name/pattern is specified

When an index containing no time fields is specified

When an index containing time fields is specified

cjcenizal commented Apr 25, 2017 • edited Loading

cjcenizal left a 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

Choose a reason for hiding this comment

ycombinator commented Apr 25, 2017

cjcenizal left a comment

Choose a reason for hiding this comment

ycombinator commented Apr 25, 2017

rhoboat commented Apr 25, 2017

w33ble left a 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

cjcenizal commented Apr 26, 2017

ycombinator commented Apr 26, 2017

ycombinator commented Apr 26, 2017

ycombinator commented Apr 25, 2017 •

edited

Loading

cjcenizal commented Apr 25, 2017 •

edited

Loading

cjcenizal commented Apr 25, 2017 •

edited

Loading