[Canvas] Converting time filter (and children) component to typescript and adding to storybook #40443

poffdeluxe · 2019-07-05T19:37:12Z

Summary

Converting all the time filter components to typescript and adding them to storybook (and storyshots) to work towards #40160. This PR uses a lot of the @cqliu1's work from #31672

Also, for storyshots, mocks the dateMath.parse function so that even if the storybook story uses the date format of "now-7d" (or something similar), the snapshot generated will be consistent and won't change.

Would also love feedback on the snapshots generated for some of the bigger components (particularly the components that uses the react-datepicker under the hood. I was considering mocking out the react-datepicker component so the whole thing isn't rendered but would love feedback/guidance here.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~

Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-07-05T20:37:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-05T21:02:55Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-07-05T21:59:47Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-08T14:35:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-08T16:17:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

poffdeluxe · 2019-07-08T17:15:14Z

retest

elasticmachine · 2019-07-08T18:15:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

poffdeluxe · 2019-07-08T20:51:41Z

retest

elasticmachine · 2019-07-08T22:20:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

clintandrewhall

Just a few nits and considerations. I'll accept to unblock, but please address them before committing.

Nice work!!

clintandrewhall · 2019-07-10T15:09:10Z

...gin_src/renderers/time_filter/components/datetime_range_absolute/datetime_range_absolute.tsx

@@ -51,5 +64,5 @@ export const DatetimeRangeAbsolute = ({ from, to, onSelect }) => (
 DatetimeRangeAbsolute.propTypes = {
  from: PropTypes.object, // a moment


Let's consider either integrating react-moment-proptypes or PropTypes.instanceof here.

clintandrewhall · 2019-07-10T15:12:44Z

.../canvas/canvas_plugin_src/renderers/time_filter/components/pretty_duration/lib/time_units.ts

@@ -4,7 +4,7 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-export const timeUnits = {
+export const timeUnits: { [key: string]: string } = {


Consider:

export enum TimeUnit { SECONDS = 's', MINUTES = 'm', // etc } export const timeUnits: Record<TimeUnit, string> = { [TimeUnit.SECONDS]: 'second', [TimeUnit.MINUTE]: 'minute', // etc }

clintandrewhall · 2019-07-10T15:14:10Z

.../plugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker_mini/index.ts

+ * you may not use this file except in compliance with the Elastic License.
+ */
+
+export { TimePickerMini } from './time_picker_mini';


nit: I've been renaming mini to popover, as that seems much more descriptive of the underlying component.

taken care of. I had to do change a css file and a template as well but it should all be switched from TimePickerMini -> TimePickerPopover

clintandrewhall · 2019-07-10T15:15:05Z

...gin_src/renderers/time_filter/components/datetime_range_absolute/datetime_range_absolute.tsx

@@ -36,9 +47,11 @@ export const DatetimeRangeAbsolute = ({ from, to, onSelect }) => (
        minDate={from}
        onValueChange={val => onSelect(from, val)}
        onSelect={val => {
+          if (!val || !to) return;


This should be formatted with braces... curious why this wasn't done for you automatically.

clintandrewhall · 2019-07-10T15:19:02Z

...nvas_plugin_src/renderers/time_filter/components/datetime_quick_list/datetime_quick_list.tsx

+  children?: ReactNode;
+}
+
+const quickRanges = [


Consider adding these as an export with TimeUnit below in some common file. We'll need to i18n both at some point.

I'm a little hesitant to making that move of the quickRanges here to a common place since there's another quickRanges specific to the PrettyDuration component as well. I can refactor them to perhaps share the same quickRanges array so that we only need to update the string for i18n in one place if you think that's worth it in this PR

I'd track a follow-up. We have a lot of instances where we have these kinds of ranges, (e.g. refresh, auto-play, etc).

Do we want them all to share the same quick ranges? I imagine refresh/autoplay would share the same set of quick range time intervals, but I think time filter should probably have a different set of date ranges from the ones used for refresh/autoplay.

Side note: While looking through the available Kibana advanced settings, I noticed there was a timepicker:quickRanges setting which we may want to consider using instead of the static quickRanges array used in the time_filter component. I've added it to my list of Kibana advanced settings we'd want to integrate into Canvas (#37436).

I agree that this doesn't need to be addressed in this PR and that we should create an issue to track it.

Tracking that here: #41055

poffdeluxe · 2019-07-10T22:38:24Z

...s/canvas_plugin_src/renderers/time_filter/components/datetime_calendar/datetime_calendar.tsx

+);
+
+DatetimeCalendar.propTypes = {
+  value: PropTypes.oneOfType([momentObj, PropTypes.object]), // Handle both valid and invalid moment objects


@clintandrewhall for moment proptypes, I went the react-moment-proptypes approach (Mostly because with typescript, Moment is the typescript type and not the moment class so the instanceOf(Moment) approach won't work unless I'm missing something). The downside is that react-moment-proptypes checks not only if the prop is a moment but it also checks if it's a valid moment which messes with the DatetimeCalendar component since it's supposed to be able to handle invalid moments.

My compromise was to make the value proptype be either a moment or an object but I don't love it. I could make a custom validator proptype for value but it'd be only for just this single prop. Any thoughts or suggestions?

elasticmachine · 2019-07-10T22:45:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

poffdeluxe · 2019-07-10T22:51:21Z

...plugins/canvas/canvas_plugin_src/renderers/time_filter/components/datetime_calendar/index.ts

@@ -4,12 +4,4 @@
 * you may not use this file except in compliance with the Elastic License.
 */

-export const timeUnits = {


weird diff here

…ing to storybook Mocking the datepicker dep to avoid rendering huge snapshots Updating moments in examples to be timezone agnostic Updating snapshots for time picker

elasticmachine · 2019-07-11T14:41:36Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

snide

Sass changes in here are just namespacing. Didn't test in browser for review, just code.

elasticmachine · 2019-07-11T18:01:23Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-11T23:08:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

...gin_src/renderers/time_filter/components/datetime_range_absolute/datetime_range_absolute.tsx

elasticmachine · 2019-07-12T15:25:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/time_picker.tsx

cqliu1

Just a few nitpicks, but overall the code looks good!

.../canvas/canvas_plugin_src/renderers/time_filter/components/datetime_input/datetime_input.tsx

...gin_src/renderers/time_filter/components/datetime_range_absolute/datetime_range_absolute.tsx

...anvas/canvas_plugin_src/renderers/time_filter/components/pretty_duration/lib/quick_ranges.ts

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_filter/time_filter.tsx

cqliu1 · 2019-07-12T16:51:25Z

...egacy/plugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/index.js

-import { TimePicker as Component } from './time_picker';
-
-export const TimePicker = compose(
-  withState('range', 'setRange', ({ from, to }) => ({ from, to })),


++ to removing all of this recompose stuff and converting the component to a class.

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/time_picker.tsx

cqliu1 · 2019-07-12T16:59:48Z

...cy/plugins/canvas/canvas_plugin_src/renderers/time_filter/components/datetime_input/index.ts

+      setStrValue(moment.format('YYYY-MM-DD HH:mm:ss'));
+      setValid(true);
+    },
+  })


Q: Any reason why you removed the recompose stuff from time_picker/index.js, but not here? Not saying you have to refactor this one, but I'd prefer converting this component to a class instead of calling lifecycle functions in the /index using recompose.

I kinda wanted to minimize this PRs footprint by only updating what needed to be updated. I removed recompose from time_picker/index.js since the typing in there with recompose was getting very hard to read. Def down for removing recompose in a future PR though

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/time_picker.tsx

elasticmachine · 2019-07-12T21:46:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…t and adding to storybook (elastic#40443) * Converting time filter (and children) component to typescript and adding to storybook

…t and adding to storybook (#40443) (#41058) * Converting time filter (and children) component to typescript and adding to storybook

poffdeluxe force-pushed the timefilter-ts branch from 5b2f7fc to 1bba246 Compare July 5, 2019 19:39

poffdeluxe added chore impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes loe:medium Medium Level of Effort labels Jul 5, 2019

poffdeluxe force-pushed the timefilter-ts branch from 1bba246 to b3e0c3e Compare July 5, 2019 21:00

poffdeluxe added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jul 5, 2019

poffdeluxe added the review label Jul 5, 2019

poffdeluxe marked this pull request as ready for review July 5, 2019 21:03

poffdeluxe requested a review from a team as a code owner July 5, 2019 21:03

poffdeluxe added v7.4.0 v8.0.0 labels Jul 5, 2019

poffdeluxe force-pushed the timefilter-ts branch from 26b5c01 to 065bf9d Compare July 8, 2019 15:18

poffdeluxe closed this Jul 8, 2019

poffdeluxe reopened this Jul 8, 2019

poffdeluxe requested a review from cqliu1 July 8, 2019 16:27

poffdeluxe requested a review from clintandrewhall July 9, 2019 16:01

clintandrewhall approved these changes Jul 10, 2019

View reviewed changes

poffdeluxe force-pushed the timefilter-ts branch from a087844 to 2179058 Compare July 10, 2019 22:28

poffdeluxe requested a review from a team as a code owner July 10, 2019 22:28

poffdeluxe commented Jul 10, 2019

View reviewed changes

poffdeluxe added 4 commits July 11, 2019 09:27

Converting time filter (and children) component to typescript and add…

9cb0670

…ing to storybook Mocking the datepicker dep to avoid rendering huge snapshots Updating moments in examples to be timezone agnostic Updating snapshots for time picker

Setting default timezone for storyshot and updating snapshots

fa651dd

Rename TimePickerMini component to TimePickerPopover

26a9d5f

other various small changes

e4f5a0c

poffdeluxe force-pushed the timefilter-ts branch from 2179058 to e4f5a0c Compare July 11, 2019 14:27

Adding react-moment-proptypes to yarn.lock

391c0f1

snide approved these changes Jul 11, 2019

View reviewed changes

poffdeluxe added 2 commits July 11, 2019 15:49

Missed one place with the time picker mini -> popover

9e01d49

Type fixing

a6419df

cqliu1 reviewed Jul 12, 2019

View reviewed changes

...gin_src/renderers/time_filter/components/datetime_range_absolute/datetime_range_absolute.tsx Outdated Show resolved Hide resolved

Linting fix

abd3bb4

cqliu1 reviewed Jul 12, 2019

View reviewed changes

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/time_picker.tsx Outdated Show resolved Hide resolved

cqliu1 reviewed Jul 12, 2019

View reviewed changes

...lugins/canvas/canvas_plugin_src/renderers/time_filter/components/time_picker/time_picker.tsx Outdated Show resolved Hide resolved

cqliu1 requested changes Jul 12, 2019

View reviewed changes

Updating per feedback

74054e1

cqliu1 approved these changes Jul 12, 2019

View reviewed changes

poffdeluxe merged commit 8be17fe into elastic:master Jul 12, 2019

poffdeluxe deleted the timefilter-ts branch July 12, 2019 21:48

poffdeluxe mentioned this pull request Jul 12, 2019

[7.x] [Canvas] Converting time filter (and children) component to typescript and adding to storybook (#40443) #41058

Merged

poffdeluxe added a commit that referenced this pull request Jul 13, 2019

[Canvas] Converting time filter (and children) component to typescrip…

5f8f85f

…t and adding to storybook (#40443) (#41058) * Converting time filter (and children) component to typescript and adding to storybook

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Canvas] Converting time filter (and children) component to typescript and adding to storybook #40443

[Canvas] Converting time filter (and children) component to typescript and adding to storybook #40443

poffdeluxe commented Jul 5, 2019 •

edited

Loading

elasticmachine commented Jul 5, 2019

elasticmachine commented Jul 5, 2019

elasticmachine commented Jul 5, 2019

elasticmachine commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

poffdeluxe commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

poffdeluxe commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

clintandrewhall left a comment

clintandrewhall Jul 10, 2019

clintandrewhall Jul 10, 2019

clintandrewhall Jul 10, 2019

poffdeluxe Jul 10, 2019

clintandrewhall Jul 10, 2019

clintandrewhall Jul 10, 2019

poffdeluxe Jul 10, 2019

clintandrewhall Jul 11, 2019

cqliu1 Jul 12, 2019 •

edited

Loading

poffdeluxe Jul 12, 2019

poffdeluxe Jul 10, 2019 •

edited

Loading

elasticmachine commented Jul 10, 2019

poffdeluxe Jul 10, 2019

elasticmachine commented Jul 11, 2019

snide left a comment

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 11, 2019

elasticmachine commented Jul 12, 2019

cqliu1 left a comment

cqliu1 Jul 12, 2019

cqliu1 Jul 12, 2019

poffdeluxe Jul 12, 2019

elasticmachine commented Jul 12, 2019

		@@ -51,5 +64,5 @@ export const DatetimeRangeAbsolute = ({ from, to, onSelect }) => (
		DatetimeRangeAbsolute.propTypes = {
		from: PropTypes.object, // a moment

[Canvas] Converting time filter (and children) component to typescript and adding to storybook #40443

[Canvas] Converting time filter (and children) component to typescript and adding to storybook #40443

Conversation

poffdeluxe commented Jul 5, 2019 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Jul 5, 2019

💚 Build Succeeded

elasticmachine commented Jul 5, 2019

elasticmachine commented Jul 5, 2019

💚 Build Succeeded

elasticmachine commented Jul 8, 2019

💔 Build Failed

elasticmachine commented Jul 8, 2019

💔 Build Failed

poffdeluxe commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

💔 Build Failed

poffdeluxe commented Jul 8, 2019

elasticmachine commented Jul 8, 2019

💚 Build Succeeded

clintandrewhall 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

cqliu1 Jul 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poffdeluxe Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jul 10, 2019

💔 Build Failed

Choose a reason for hiding this comment

elasticmachine commented Jul 11, 2019

💔 Build Failed

snide left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 11, 2019

💔 Build Failed

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

elasticmachine commented Jul 12, 2019

💚 Build Succeeded

cqliu1 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

elasticmachine commented Jul 12, 2019

💚 Build Succeeded

poffdeluxe commented Jul 5, 2019 •

edited

Loading

cqliu1 Jul 12, 2019 •

edited

Loading

poffdeluxe Jul 10, 2019 •

edited

Loading