[Lens] Make date histogram interval more customizable #49217

chrisdavies · 2019-10-24T16:37:10Z

The date histogram no longer uses a slider for customizing the interval. It's a combination of a numeric input plus a unit dropdown:

When switching from auto, the initial value is automatically determined using the same interval as the auto generation.

The error state is similar to visualize's, which is pretty generic. Eventually, it'd be nice to provide the user with more helpful information:

[Note: the example screenshot should not be an error state... fixing that in a subsequent commit.]

…ime-intervals

elasticmachine · 2019-10-24T17:59:29Z

💔 Build Failed

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

elasticmachine · 2019-10-24T19:46:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 6e6ea5d

brianseeders · 2019-10-24T19:48:21Z

retest

elasticmachine · 2019-10-24T21:16:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 6e6ea5d

…ime-intervals

elasticmachine · 2019-10-25T16:29:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8b83337

wylieconlon

I haven't tested this locally, but the pieces here make sense to me. I think there's a conflict with master because I changed the text in one place you did too.

wylieconlon · 2019-10-25T17:37:47Z

x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx

@@ -7,31 +7,26 @@
 import React from 'react';
 import { i18n } from '@kbn/i18n';
 import { FormattedMessage } from '@kbn/i18n/react';
-import { EuiForm, EuiFormRow, EuiRange, EuiSwitch } from '@elastic/eui';
+import { isValidInterval } from 'ui/agg_types/utils';


Imports from ui/<something> won't be possible in the new platform, maybe this needs to be passed in from the plugin?

wylieconlon · 2019-10-25T17:38:32Z

...egacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.test.tsx

+          },
+        },
+      },
+    } as unknown) as IndexPatternPrivateState;


lol. I'm open to alternatives. This is an example of where TypeScript isn't super helpful.

wylieconlon · 2019-10-25T17:38:45Z

...egacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.test.tsx

@@ -217,7 +252,7 @@ describe('date_histogram', () => {
      expect(column.label).toContain('start_date');
    });

-    it('should change interval from auto when switching to a non primary time field', () => {
+    it('should not change interval from auto when switching to a non primary time field', () => {


wylieconlon · 2019-10-25T17:45:52Z

...egacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.test.tsx

+          uiSettings={{} as UiSettingsClientContract}
+          savedObjectsClient={{} as SavedObjectsClientContract}
+          dateRange={dateRange}
+          http={{} as HttpServiceBase}


Might be good to do the same {...defaultOptions} as below

elasticmachine · 2019-10-25T20:03:03Z

💔 Build Failed

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

chrisdavies · 2019-10-25T20:22:00Z

retest

elasticmachine · 2019-10-25T21:40:00Z

💚 Build Succeeded

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

chrisdavies · 2019-10-28T13:10:03Z

@elasticmachine merge upstream

elasticmachine · 2019-10-28T14:29:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5018fe3

wylieconlon

Still haven't tested locally but LGTM

wylieconlon · 2019-10-28T19:41:45Z

x-pack/legacy/plugins/lens/public/editor_frame_plugin/editor_frame/editor_frame.test.tsx

      mockDatasource.initialize.mockResolvedValue(datasourceState);
      mockDatasource.getLayers.mockReturnValue(['first']);

      mount(
        <EditorFrame
-          {...getDefaultProps()}
+          {...{ ...getDefaultProps(), dateRange }}


Isn't this the same as {...getDefaultProps()} dateRange={dateRange}?

Yup. I'll change it.

timroes

Overall looks good to me. Have one concern regarding typing and one regarding wordings.

timroes · 2019-10-29T09:38:29Z

x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx

+    // isValidInterval involves breaking changes in other areas.
+    const isValid = isValidInterval(
+      `${interval.value === '' ? '1' : interval.value}${interval.unit}`,
+      restrictedInterval(field!.aggregationRestrictions) as string


We always cast the actual return type string | undefined here to a string when we use this. As someone not that familiar with the code, it's not clear to me why we could do that? Why do we know that that method will always return a string, even though it could also return undefined? Is there potentially a better way to type that? If not, can we at least leave a comment, why we know this can never return undefined, or build an assert into that restrictedInterval function that will throw if it ever would be (and TS would then properly detect that it's only a string)?

It's because the type signature for isValidInterval is wrong. It requires a string, but handles undefined explicitly and properly. I just didn't want to have a post-feature-freeze PR affect code outside of Lens, if possible, so I didn't fix the type signature.

[Edit: I'll fix the type signature!]

timroes · 2019-10-29T09:42:23Z

x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx

@@ -197,7 +190,7 @@ export const dateHistogramOperation: OperationDefinition<DateHistogramIndexPatte
        {currentColumn.params.interval !== autoInterval && (
          <EuiFormRow
            label={i18n.translate('xpack.lens.indexPattern.dateHistogram.interval', {
-              defaultMessage: 'Time intervals',
+              defaultMessage: 'Time interval',


We should use the same wording here as in the old editor here, since we don't actually guarantee that we will use that exact interval, but this interval or an interval larger than that (in case we would generate too many buckets otherwise). We've seen in the old editor that users were often confused before the wording change, why they configure a time interval and we use a different one nevertheless.

…ibana into lens/custom-time-intervals

elasticmachine · 2019-10-29T16:35:46Z

💚 Build Succeeded

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

Make lens date histogram interval more customizable

0d03e69

chrisdavies added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.5.0 labels Oct 24, 2019

chrisdavies requested review from wylieconlon and a team October 24, 2019 16:37

chrisdavies added 3 commits October 24, 2019 12:37

Rename time intervals to time interval

fdbdddd

Merge branch 'master' of github.com:elastic/kibana into lens/custom-t…

95cecce

…ime-intervals

Fix interval validation bug

6e6ea5d

wylieconlon added the v7.6.0 label Oct 24, 2019

chrisdavies added 2 commits October 25, 2019 10:45

Merge branch 'master' of github.com:elastic/kibana into lens/custom-t…

4e177f2

…ime-intervals

Fix TypeScript errors

8b83337

wylieconlon approved these changes Oct 25, 2019

View reviewed changes

chrisdavies added 2 commits October 25, 2019 14:40

Tweaks per Wylie's feedback

418c9a2

Merge master

d628075

Merge branch 'master' into lens/custom-time-intervals

5018fe3

wylieconlon approved these changes Oct 28, 2019

View reviewed changes

timroes reviewed Oct 29, 2019

View reviewed changes

chrisdavies added 4 commits October 29, 2019 11:01

Merge master

9e889e5

Fix TypeScript errors

2596a0a

Tweaks per PR feedback

b1e5f13

Merge branch 'lens/custom-time-intervals' of github.com:chrisdavies/k…

edeeefe

…ibana into lens/custom-time-intervals

chrisdavies merged commit ebb4944 into elastic:master Oct 29, 2019

chrisdavies deleted the lens/custom-time-intervals branch October 29, 2019 16:48

This was referenced Oct 29, 2019

[7.x] [Lens] Make date histogram interval more customizable (#49217) #49654

Merged

[7.5] [Lens] Make date histogram interval more customizable (#49217) #49655

Merged

chrisdavies added a commit that referenced this pull request Oct 29, 2019

[Lens] Make date histogram interval more customizable (#49217) (#49654)

f90855e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Make date histogram interval more customizable #49217

[Lens] Make date histogram interval more customizable #49217

chrisdavies commented Oct 24, 2019 •

edited

Loading

elasticmachine commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

brianseeders commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

elasticmachine commented Oct 25, 2019

wylieconlon left a comment

wylieconlon Oct 25, 2019

wylieconlon Oct 25, 2019

chrisdavies Oct 25, 2019

wylieconlon Oct 25, 2019

wylieconlon Oct 25, 2019

elasticmachine commented Oct 25, 2019

chrisdavies commented Oct 25, 2019

elasticmachine commented Oct 25, 2019

chrisdavies commented Oct 28, 2019

elasticmachine commented Oct 28, 2019

wylieconlon left a comment

wylieconlon Oct 28, 2019

chrisdavies Oct 29, 2019

timroes left a comment

timroes Oct 29, 2019

chrisdavies Oct 29, 2019 •

edited

Loading

timroes Oct 29, 2019

elasticmachine commented Oct 29, 2019

[Lens] Make date histogram interval more customizable #49217

[Lens] Make date histogram interval more customizable #49217

Conversation

chrisdavies commented Oct 24, 2019 • edited Loading

elasticmachine commented Oct 24, 2019

💔 Build Failed

elasticmachine commented Oct 24, 2019

💔 Build Failed

brianseeders commented Oct 24, 2019

elasticmachine commented Oct 24, 2019

💔 Build Failed

elasticmachine commented Oct 25, 2019

💚 Build Succeeded

wylieconlon 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

elasticmachine commented Oct 25, 2019

💔 Build Failed

chrisdavies commented Oct 25, 2019

elasticmachine commented Oct 25, 2019

💚 Build Succeeded

chrisdavies commented Oct 28, 2019

elasticmachine commented Oct 28, 2019

💚 Build Succeeded

wylieconlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdavies Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 29, 2019

💚 Build Succeeded

chrisdavies commented Oct 24, 2019 •

edited

Loading

chrisdavies Oct 29, 2019 •

edited

Loading