[ML] New job wizards UI by jgowdyelastic · Pull Request #40748 · elastic/kibana

jgowdyelastic · 2019-07-10T13:11:26Z

First PR for the jobs wizard remake UI.
In this initial version, single metric, multi metric and population jobs can be created.
For multi metric and population jobs, the data can be split in a similar way to the original wizards.

As with the original wizards, when starting the jobs the results will be polled and drawn over the charts.

Basic things missing in this PR which will be added in follow ups:

internationalisation - there are very few translated strings
tests - there are none
replace placeholder text for all form inputs

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-10T14:11:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T15:47:54Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T17:46:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T18:20:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-10T18:52:18Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-11T11:52:37Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

walterra

LGTM overall, just a bunch of questions/thoughts.

walterra · 2019-07-11T12:12:27Z

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/chart_settings.ts


-    // @ts-ignore: incomplete kibana types
-    const bounds = timefilter.getActiveBounds();
+    const tf = timefilter as any;


Once Melissa's navbar PR is in this shouldn't be necessary anymore if you merge master then, she added the missing types to timefilter.

https://github.com/elastic/kibana/projects/30#card-23856937

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/results_loader/results_loader.ts

..._job_new/pages/components/pick_fields_step/components/multi_metric_view/metric_selection.tsx

walterra · 2019-07-11T12:44:00Z

...job_new/pages/components/pick_fields_step/components/multi_metric_view/multi_metric_view.tsx

+      {metricsValid && isActive && (
+        <Fragment>
+          <EuiHorizontalRule margin="l" />
+          <MultiMetricSettings isActive={isActive} setIsValid={setSettingsValid} />


ultra-semantics-nit: since setIsValid is the callback inside the component and not the setter itself, what if these kind of props were called onIsValid (similar to onChange exposed by EUI components or native listeners).

onIsValid doesn't describe this as well as setIsValid IMO.
The prop is allowing the component to report whether it is valid. something like onSettingsChange or something like that would be more accurate, but then it loses the point that it's for validation. I'm going to leave this name as is, although it's not the great, it's better than alternatives.

walterra · 2019-07-11T13:22:09Z

...ic/jobs/new_job_new/pages/components/pick_fields_step/components/split_cards/split_cards.tsx

+          marginBottom: `-${SPACING}px`,
+          marginLeft: `${sideMargin}px`,
+          marginRight: `${sideMargin}px`,
+          transition: 'margin 0.2s',


does this need cross-browser alternatives since it's not running through SCSS compilation?

Good point, I think it should probably be moved to SCSS.
I'll look into that in a follow up https://github.com/elastic/kibana/projects/30#card-23885080

Here is how the cards split on IE11:

...ublic/jobs/new_job_new/pages/components/pick_fields_step/components/split_field/by_field.tsx

walterra · 2019-07-11T13:46:03Z

x-pack/legacy/plugins/ml/public/jobs/new_job_new/pages/new_job/page.tsx

+          <EuiPageContentBody>
+            <EuiSpacer size="l" />
+            <Wizard
+              jobCreator={jobCreator}


Could jobCreatorContext already be created in this component and be applied in the jsx here using <JobCreatorContext.Provider value={jobCreatorContext}>? Then it wouldn't be necessary to pass it down here as a prop and the code to create it wouldn't be spread across 2 files.

possibly yes, i think i tried that but hit a problem. i'll revisit it.

I remember now, the creation of the jobCreator needs to be in a higher level component otherwise it gets recreated on re-render.
The jobCreator, chartInterval and chartLoader classes could be created in a separate factory function above the page component.
This whole structure can be revisited later in a refactor.

https://github.com/elastic/kibana/projects/30#card-23887483

walterra · 2019-07-11T13:49:57Z

x-pack/legacy/plugins/ml/public/services/ml_api_service/jobs.js

    });
  },
+
+  newJobLineChart(


could we just pass a data object then we wouldn't have to list all the args/properties twice? (guess this might make sense once migrating to typescript because then the data object could be defined by an interface instead of having single functions args here)

This can be addressed when we typescriptify the ml_api_service file.

peteharverson

Overall LGTM. Just some minor comments and questions.
Is adding data-test-subj attributes on the TODO list too?

x-pack/legacy/plugins/ml/common/util/anomaly_utils.js

peteharverson · 2019-07-11T15:20:06Z

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/chart_loader/chart_loader.ts

+        this._timeFieldName,
+        start,
+        end,
+        intervalMs * 3


Why is 3 being used here? Could it be moved to a const with a comment?

Short answer is it makes the chart look nicer by widening the buckets.
I'll see if there is a nicer way to do it, or make it a const

https://github.com/elastic/kibana/projects/30#card-23887507

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/results_loader/results_loader.ts

peteharverson · 2019-07-11T15:29:06Z

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/results_loader/searches.ts

+      jobIdFilterStr += `"${String(firstSplitField.value).replace(/\\/g, '\\\\')}"`;
+    }
+
+    ml.esSearch({


Can this search be moved to the server-side results_service?

yes hopefully that can happen in a later PR. I'd like all searches to happen server side

https://github.com/elastic/kibana/projects/30#card-23855269

x-pack/legacy/plugins/ml/public/jobs/new_job_new/pages/components/charts/common/settings.ts

x-pack/legacy/plugins/ml/public/jobs/new_job_new/pages/new_job/route.ts

x-pack/legacy/plugins/ml/public/util/ml_time_buckets.d.ts

x-pack/legacy/plugins/ml/server/models/job_service/new_job/line_chart.ts

x-pack/legacy/plugins/ml/server/models/job_service/new_job/population_chart.ts

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/chart_loader/chart_loader.ts

peteharverson · 2019-07-11T16:33:46Z

Is adding in the created_by prop into the job's custom_settings on the TODO list?

jgowdyelastic · 2019-07-11T16:39:03Z

Is adding in the created_by prop into the job's custom_settings on the TODO list?

it is now.

alvarezmelissa87 · 2019-07-11T17:53:21Z

Overall, LGTM - the only thing I'd feel more strongly about are the places Walter pointed out that the subscription to the observable could be returned so that on unmount unsubscribe can be called on it. (when using jobCreator.subscribe... and resultsLoader.subscribe..
Sounds like it would be fine to be done in a follow-up as you've already got some planned.

elasticmachine · 2019-07-11T18:21:31Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-12T10:37:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

peteharverson

Latest changes and comments LGTM

elasticmachine · 2019-07-12T11:56:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-12T12:50:33Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alvarezmelissa87

Latest changes LGTM ⚡️

alvarezmelissa87 · 2019-07-11T17:32:35Z

...s/new_job_new/pages/components/pick_fields_step/components/detector_title/detector_title.tsx

+};
+
+function getTitle(agg: Aggregation, field: Field, splitField: SplitField): string {
+  // let title = ${agg.title}(${field.name})`;


Is this ever going to be needed?

This was raised by Pete also #40748 (comment)
I'm going to leave it in for now as there is outstanding work on the detector titles

* [ML] Adding new job wizards services (#40718) * [ML] Adding new job wizards services * adding ts-ignore for incomplte kibana timefilter type * [ML] New job wizards UI (#40748) * [ML] New job wizards UI * small refactor * fixing type problems * updating snapshots * updating translations ids for temporary new job links * updating translation ids * fixing population by field selection * small refactor to by field select * moving component descriptions to separate files * moving chart grid component into own file * fixing event rate count detector * fixing decimal place rounding * changes based on review * more changes based on review * fixing test * [ML] Adding created_by to job (#41015) * [ML] Adding json preview (#41078) * [ML] Adding job json preview * fix typo * [ML] Fixing view results button (#41077) * [ML] Adding generic custom settings functions to job creator (#41046) * [ML] Adding generic custom settings functions to job creator * fixing typo * [ML] Adding advanced section to job details step (#41157) * [ML] Adding advanced section to job details step * disabling model plot for population * changes based on review * updating include paths * [ML] Adding additional settings section to job details step (#41339) * [ML] Adding additional settings section to job details step * changes based on review * [ML] Adding basic job validation (#41459) * [ML] Adding basic job validation * removing observable * small refactor * comments and refactors * disabling create job button * adding duplicate job and group id checks * disabling next on invalid wizard step * changes based on review * removing unused includes * [ML] Adding advanced job validation (#41634) * [ML] Adding advanced job validation * clean up * function rename * updating tests * show some previous step info * removing . from validation titles * fixes advanced validation duration check * removing include * [ML] Changing wizard layout (#41802) * [ML] Changing wizard layout * adding a comment * fixing wizard step link issue * removing greyed out controls in summary * filtering out basic validation results * adding wizards steps back into summary * moving idFilterList * removing unused includes * [ML] Adding date picker to time range step (#41978) * [ML] Adding date picker to time range step * adding comments * sorting influencers * removing euiSize for width * [ML] Displaying job creation errors (#42165) * [ML] Displaying job creation errors * removing unnecessary try/catch * [ML] Updating index pattern type (#42269) * [ML] Updating index pattern type * changes based on review

* [ML] Adding new job wizards services (elastic#40718) * [ML] Adding new job wizards services * adding ts-ignore for incomplte kibana timefilter type * [ML] New job wizards UI (elastic#40748) * [ML] New job wizards UI * small refactor * fixing type problems * updating snapshots * updating translations ids for temporary new job links * updating translation ids * fixing population by field selection * small refactor to by field select * moving component descriptions to separate files * moving chart grid component into own file * fixing event rate count detector * fixing decimal place rounding * changes based on review * more changes based on review * fixing test * [ML] Adding created_by to job (elastic#41015) * [ML] Adding json preview (elastic#41078) * [ML] Adding job json preview * fix typo * [ML] Fixing view results button (elastic#41077) * [ML] Adding generic custom settings functions to job creator (elastic#41046) * [ML] Adding generic custom settings functions to job creator * fixing typo * [ML] Adding advanced section to job details step (elastic#41157) * [ML] Adding advanced section to job details step * disabling model plot for population * changes based on review * updating include paths * [ML] Adding additional settings section to job details step (elastic#41339) * [ML] Adding additional settings section to job details step * changes based on review * [ML] Adding basic job validation (elastic#41459) * [ML] Adding basic job validation * removing observable * small refactor * comments and refactors * disabling create job button * adding duplicate job and group id checks * disabling next on invalid wizard step * changes based on review * removing unused includes * [ML] Adding advanced job validation (elastic#41634) * [ML] Adding advanced job validation * clean up * function rename * updating tests * show some previous step info * removing . from validation titles * fixes advanced validation duration check * removing include * [ML] Changing wizard layout (elastic#41802) * [ML] Changing wizard layout * adding a comment * fixing wizard step link issue * removing greyed out controls in summary * filtering out basic validation results * adding wizards steps back into summary * moving idFilterList * removing unused includes * [ML] Adding date picker to time range step (elastic#41978) * [ML] Adding date picker to time range step * adding comments * sorting influencers * removing euiSize for width * [ML] Displaying job creation errors (elastic#42165) * [ML] Displaying job creation errors * removing unnecessary try/catch * [ML] Updating index pattern type (elastic#42269) * [ML] Updating index pattern type * changes based on review

* [ML] Adding new job wizards services (#40718) * [ML] Adding new job wizards services * adding ts-ignore for incomplte kibana timefilter type * [ML] New job wizards UI (#40748) * [ML] New job wizards UI * small refactor * fixing type problems * updating snapshots * updating translations ids for temporary new job links * updating translation ids * fixing population by field selection * small refactor to by field select * moving component descriptions to separate files * moving chart grid component into own file * fixing event rate count detector * fixing decimal place rounding * changes based on review * more changes based on review * fixing test * [ML] Adding created_by to job (#41015) * [ML] Adding json preview (#41078) * [ML] Adding job json preview * fix typo * [ML] Fixing view results button (#41077) * [ML] Adding generic custom settings functions to job creator (#41046) * [ML] Adding generic custom settings functions to job creator * fixing typo * [ML] Adding advanced section to job details step (#41157) * [ML] Adding advanced section to job details step * disabling model plot for population * changes based on review * updating include paths * [ML] Adding additional settings section to job details step (#41339) * [ML] Adding additional settings section to job details step * changes based on review * [ML] Adding basic job validation (#41459) * [ML] Adding basic job validation * removing observable * small refactor * comments and refactors * disabling create job button * adding duplicate job and group id checks * disabling next on invalid wizard step * changes based on review * removing unused includes * [ML] Adding advanced job validation (#41634) * [ML] Adding advanced job validation * clean up * function rename * updating tests * show some previous step info * removing . from validation titles * fixes advanced validation duration check * removing include * [ML] Changing wizard layout (#41802) * [ML] Changing wizard layout * adding a comment * fixing wizard step link issue * removing greyed out controls in summary * filtering out basic validation results * adding wizards steps back into summary * moving idFilterList * removing unused includes * [ML] Adding date picker to time range step (#41978) * [ML] Adding date picker to time range step * adding comments * sorting influencers * removing euiSize for width * [ML] Displaying job creation errors (#42165) * [ML] Displaying job creation errors * removing unnecessary try/catch * [ML] Updating index pattern type (#42269) * [ML] Updating index pattern type * changes based on review

jgowdyelastic added 2 commits July 10, 2019 13:59

[ML] New job wizards UI

dc9775b

small refactor

16b1c02

jgowdyelastic requested a review from a team as a code owner July 10, 2019 13:11

jgowdyelastic added 4 commits July 10, 2019 15:50

fixing type problems

3e9edbb

updating snapshots

8b82370

updating translations ids for temporary new job links

7aa7173

updating translation ids

d1f77c2

fixing population by field selection

9ee2442

small refactor to by field select

693b5b1

jgowdyelastic added 2 commits July 11, 2019 11:37

moving component descriptions to separate files

fbde12b

moving chart grid component into own file

6dd0ce6

walterra approved these changes Jul 11, 2019

View reviewed changes

jgowdyelastic added 2 commits July 11, 2019 16:44

fixing event rate count detector

6ab2970

fixing decimal place rounding

c084e88

peteharverson reviewed Jul 11, 2019

View reviewed changes

alvarezmelissa87 reviewed Jul 11, 2019

View reviewed changes

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/chart_loader/chart_loader.ts Outdated Show resolved Hide resolved

changes based on review

c11cbef

more changes based on review

9f91250

peteharverson approved these changes Jul 12, 2019

View reviewed changes

fixing test

f78e4b7

alvarezmelissa87 approved these changes Jul 12, 2019

View reviewed changes

jgowdyelastic merged commit 35a4dc5 into elastic:feature-new-ml-job-wizards-new Jul 12, 2019

jgowdyelastic mentioned this pull request Jul 12, 2019

[ML] [WIP] New job wizards UI #38987

Closed

7 tasks

Conversation

jgowdyelastic commented Jul 10, 2019 • edited by peteharverson Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

For maintainers

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jul 10, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 11, 2019

💚 Build Succeeded

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgowdyelastic commented Jul 10, 2019 •

edited by peteharverson

Loading

jgowdyelastic Jul 12, 2019 •

edited

Loading