[Logs UI] move ML job setup UI to a flyout by afgomez · Pull Request #68366 · elastic/kibana

afgomez · 2020-06-05T12:15:02Z

Summary

Move the setup of log entry rate and categorization jobs to a flyout.

Checklist

Delete any items that are not applicable to this PR.

This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser

weltenwort

As mentioned on a different channel, the overall direction looks good. I left a few thoughts on state and component hierarchy below.

x-pack/plugins/infra/common/log_analysis/log_analysis.ts

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_content.tsx

Render the flyout only in the setup and the results components, controlling the visibility locally.

elasticmachine · 2020-06-24T15:14:24Z

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

weltenwort

👀 Reviewing on behalf of @elastic/logs-metrics-ui...

weltenwort

This is pretty readable 👍 I left some comments about the text and the flyout visibility state below.

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_content.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_results_content.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/setup_flyout.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/setup_flyout.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx

…ge_setup_content.tsx Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

x-pack/plugins/infra/public/components/logging/log_analysis_setup/process_step/process_step.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx

weltenwort

This seems to work pretty well now 👏

I noticed one difference between the log rate and categories tabs in that the flyout is not opened automatically in the log rate tab when the setup status indicates that the jobs are missing. Is that intentional?

I left a suggestion about improving the post-setup success state quirk in an earlier comment already.

Otherwise I think we're pretty close to done on this one after the tests pass and the text is smoothed out.

x-pack/plugins/infra/public/pages/logs/log_entry_rate/setup_flyout.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_rate/page_setup_content.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_content.tsx

x-pack/plugins/infra/public/components/logging/log_analysis_setup/process_step/process_step.tsx

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx

afgomez · 2020-07-03T13:28:05Z

@elasticmachine merge upstream

afgomez · 2020-07-06T08:50:39Z

@elasticmachine merge upstream

weltenwort

The post-setup state makes sense now as well 👍

One thing I commented on earlier still occurs: The flyout is not opened automatically in the log rate tab when the setup status indicates that the jobs are missing. Is that an intentional difference between the log rate and categories tab?

afgomez · 2020-07-06T11:10:49Z

The flyout is not opened automatically in the log rate tab

That's a mistake I missed. I'll fix it straight away

weltenwort

Thanks for polishing it so much and making it robust - good job 👏

kibanamachine · 2020-07-06T14:53:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 9db197a

Build metrics

✅ unchanged

History

💚 Build #58894 succeeded 949a5cb
💚 Build #58844 succeeded 4bf67ab
💚 Build #58669 succeeded 42a5711
💔 Build #58325 failed 189d322
💔 Build #58310 failed ab89aa4

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

Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

afgomez added v8.0.0 Feature:Logs UI Logs UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.9.0 labels Jun 5, 2020

afgomez force-pushed the 64754-ml-setup-flyout branch 2 times, most recently from 706c054 to 62ac6d2 Compare June 8, 2020 12:25

Alejandro Fernández Gómez added 3 commits June 8, 2020 18:50

Extract log entry setup to a flyout

472ec6c

Extract log entry category setup into a flyout

69ca0c6

Remove unused translations

1b89fd2

afgomez force-pushed the 64754-ml-setup-flyout branch from 62ac6d2 to 1b89fd2 Compare June 8, 2020 17:10

Alejandro Fernández Gómez added 2 commits June 10, 2020 14:35

Open setup flyout from anywhere

2de4c51

Open log categorization setup flyout from anywhere

f3b80b3

afgomez requested a review from weltenwort June 10, 2020 13:01

weltenwort reviewed Jun 10, 2020

View reviewed changes

x-pack/plugins/infra/common/log_analysis/log_analysis.ts Outdated Show resolved Hide resolved

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_content.tsx Outdated Show resolved Hide resolved

Move flyout to components where it makes sense

48577a9

Render the flyout only in the setup and the results components, controlling the visibility locally.

sgrodzicki added this to the Logs UI 7.9 milestone Jun 24, 2020

Alejandro Fernández Gómez added 2 commits June 24, 2020 16:17

Merge branch 'master' into 64754-ml-setup-flyout

b78a343

Add copy for setup pages

c6f7590

afgomez marked this pull request as ready for review June 24, 2020 15:14

afgomez requested a review from a team as a code owner June 24, 2020 15:14

afgomez added release_note:enhancement release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Jun 24, 2020

afgomez requested a review from weltenwort June 25, 2020 08:25

weltenwort reviewed Jun 25, 2020

View reviewed changes

weltenwort requested changes Jun 26, 2020

View reviewed changes

weltenwort assigned afgomez Jun 26, 2020

afgomez and others added 2 commits June 30, 2020 11:00

Update x-pack/plugins/infra/public/pages/logs/log_entry_categories/pa…

12b9f57

…ge_setup_content.tsx Co-authored-by: Felix Stürmer <weltenwort@users.noreply.github.com>

Use default flyout size

8e5fe3e

Alejandro Fernández Gómez added 4 commits June 30, 2020 11:40

Rename flyout callbacks

f24b702

Merge branch 'master' into 64754-ml-setup-flyout

2f52783

Tweak casing

e4363bb

Move flyout state one component higher

294300c

afgomez requested a review from weltenwort June 30, 2020 10:32

fixup! Move flyout state one component higher

13988e0

weltenwort reviewed Jun 30, 2020

View reviewed changes

x-pack/plugins/infra/public/components/logging/log_analysis_setup/process_step/process_step.tsx Outdated Show resolved Hide resolved

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx Outdated Show resolved Hide resolved

weltenwort reviewed Jun 30, 2020

View reviewed changes

bmorelli25 reviewed Jun 30, 2020

View reviewed changes

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx Outdated Show resolved Hide resolved

x-pack/plugins/infra/public/pages/logs/log_entry_categories/page_setup_content.tsx Outdated Show resolved Hide resolved

Alejandro Fernández Gómez added 4 commits July 1, 2020 14:54

Tweak copy

ce97c2a

Move viewResults callback composition one component higher

99292b8

Tweak translation keys

98e6d47

Tweak UI user flow after a successful job creation

ab89aa4

afgomez requested a review from weltenwort July 2, 2020 15:32

Fix tests

189d322

Merge branch 'master' into 64754-ml-setup-flyout

42a5711

Merge branch 'master' into 64754-ml-setup-flyout

4bf67ab

weltenwort reviewed Jul 6, 2020

View reviewed changes

Alejandro Fernández Gómez added 2 commits July 6, 2020 13:34

Open flyout on log rate page

949a5cb

Tweak condition to show the flyout on initial setup

9db197a

weltenwort approved these changes Jul 6, 2020

View reviewed changes

afgomez merged commit b172b5b into elastic:master Jul 6, 2020

afgomez deleted the 64754-ml-setup-flyout branch July 6, 2020 15:04

afgomez mentioned this pull request Jul 6, 2020

[7.x] [Logs UI] move ML job setup UI to a flyout (#68366) #70823

Merged

Conversation

afgomez commented Jun 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Jun 24, 2020

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afgomez commented Jul 3, 2020

Uh oh!

afgomez commented Jul 6, 2020

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

afgomez commented Jul 6, 2020

Uh oh!

weltenwort left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jul 6, 2020

💚 Build Succeeded

Build metrics

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

afgomez commented Jun 5, 2020 •

edited

Loading