Skip to content

[ML] DF Analytics creation wizard: replace select input with job type cards with icons #78872

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Sep 30, 2020
Merged

[ML] DF Analytics creation wizard: replace select input with job type cards with icons #78872

merged 4 commits into from
Sep 30, 2020

Conversation

@alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Sep 29, 2020 •
edited
Loading

Summary

Replaces job type input with cards with job type icons.

image

Checklist

Delete any items that are not applicable to this PR.

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner September 29, 2020 20:57
@alvarezmelissa87 alvarezmelissa87 self-assigned this Sep 29, 2020
@elasticmachine
Copy link
Contributor

elasticmachine commented Sep 29, 2020

Pinging @elastic/ml-ui (:ml)

@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Sep 29, 2020
Closed
60 tasks
peteharverson
peteharverson reviewed Sep 30, 2020
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the new icons here! Just left a couple of comments.

...ion/data_frame_analytics/pages/analytics_creation/components/configuration_step/job_type.tsx Outdated
Copy link
Contributor

@peteharverson peteharverson Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using grow={1} might be better here (or any alternative which guarantees that the all the cards are the same width). This would guarantee equal width of the three cards. Here the text for each is roughly the same length, so the cards are roughly the same width, but this might not be the case depending on the locale.

image

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point - updated in 3c51fbaa5146278c93c81afbd2ca5f007cabb449

...ion/data_frame_analytics/pages/analytics_creation/components/configuration_step/job_type.tsx Outdated
Copy link
Contributor

@peteharverson peteharverson Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These card titles should be internationalized (looks like they aren't used in the onClick handling).

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in 3c51fbaa5146278c93c81afbd2ca5f007cabb449

qn895
qn895 reviewed Sep 30, 2020
View reviewed changes
...ion/data_frame_analytics/pages/analytics_creation/components/configuration_step/job_type.tsx Outdated
Copy link
Member

@qn895 qn895 Sep 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use [key: DataFrameAnalysisConfigType] here to be more explicit.

Copy link
Contributor Author

@alvarezmelissa87 alvarezmelissa87 Sep 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - updated types in 3c51fbaa5146278c93c81afbd2ca5f007cabb449

@alvarezmelissa87
Copy link
Contributor Author

alvarezmelissa87 commented Sep 30, 2020

Thanks for taking a look! Updated with suggestions - this is ready for a final look when you get a chance. 🙏 cc @peteharverson, @qn895

@qn895
Copy link
Member

qn895 commented Sep 30, 2020

Code LGTM 🎉

peteharverson
peteharverson approved these changes Sep 30, 2020
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested latest changes and LGTM

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-job-type-icons branch from 3c51fba to f8e8cca Compare September 30, 2020 15:53
@kibanamachine
Copy link
Contributor

kibanamachine commented Sep 30, 2020

💚 Build Succeeded

Metrics [docs]

async chunks size

id value diff baseline
ml 10.5MB +4.7KB 10.5MB

History

  • 💔 Build #78341 failed 3c51fbaa5146278c93c81afbd2ca5f007cabb449
  • 💚 Build #78125 succeeded 23c348554305c345fe17734e7ea948f494d58067
  • 💔 Build #78096 failed 03b99b6293ef59a932b80143a7992ae5be8a6a6e
  • 💔 Build #78115 failed e82d8174fcd6aab6f0e9bd61e7e6dda761f62d54

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alvarezmelissa87 alvarezmelissa87 merged commit 5ed8fef into elastic:master Sep 30, 2020
@alvarezmelissa87 alvarezmelissa87 mentioned this pull request Sep 30, 2020
Merged
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-job-type-icons branch September 30, 2020 19:35
phillipb added a commit to phillipb/kibana that referenced this pull request Sep 30, 2020
@phillipb
Merge branch 'master' of github.com:elastic/kibana into override-anom…
1587918 
…aly-detection-partition-field

* 'master' of github.com:elastic/kibana: (37 commits)
  Fixes for the Ticket 78375 (elastic#79004)
  [Security] Alert Telemetry for the Security app (elastic#77200)
  [Search bar] Remove duplicate `popoverProps` (elastic#79025)
  [Security Solution][Detections] Add rule overrides for single event EQL rules (elastic#78876)
  [SECURITY_SOLUTION][ENDPOINT] Improve Endpoint Host data generator to also integrate with Ingest (elastic#74305)
  remove file accidentally checked in (elastic#79005)
  [ML] DF Analytics creation wizard: replace select input with job type cards with icons (elastic#78872)
  [Design] A couple fixes for 7.10 (elastic#78801)
  Fix KQL autocomplete value suggestions (elastic#78676)
  [Security Solution][Resolver] New mock with cursor (elastic#78863)
  Embeddables: basic documentation (elastic#78900)
  [security solution] only import beat_schema when needed (elastic#78708)
  [Reporting] API Integration tests: fix flaky tests for Spaces CSV formatting (elastic#78849)
  [Actions] Adds a "Test Connector" button on the Connectors List to make discovery of the Test tab easier (elastic#78746)
  [Discover] Fix functional time picker test permissions (elastic#78564)
  [ML] Fixing module datafeed overrides (elastic#78925)
  Adds some missing licenses to the CSV export (elastic#78719)
  [dev/cli] ensure plugins/ and all watch source dirs exist (elastic#78973)
  [Lens] Stop using scripted metric to collect telemetry (elastic#78687)
  [Lens] fix wrong message in fields accordion (elastic#78924)
  ...
alvarezmelissa87 added a commit that referenced this pull request Sep 30, 2020
@alvarezmelissa87
[ML] DF Analytics creation wizard: replace select input with job type…
91a7150 
… cards with icons (#78872) (#79018)

* replace select input with job type cards with icons

* update functional tests

* remove unusued translations

* ensure jobType cards remain same size and add translations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@peteharverson peteharverson peteharverson approved these changes

@qn895 qn895 qn895 approved these changes

@darnautov darnautov Awaiting requested review from darnautov

Assignees

@alvarezmelissa87 alvarezmelissa87

Labels

Feature:Data Frame Analytics ML data frame analytics features :ml release_note:enhancement v7.10.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alvarezmelissa87 @elasticmachine @qn895 @kibanamachine @peteharverson