Skip to content
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.

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

[ML][UX]: Consistent Layout and UI Enhancements for ML Pages #203813

Merged
merged 28 commits into from
Dec 23, 2024

Conversation

rbrtj
Copy link
Contributor

@rbrtj rbrtj commented Dec 11, 2024

Summary

Exploration around new Add to actions buttons - the right column is the most recent one, see: #184109 :

Before After (add_to button) After (icon button) - current
Screenshot 2024-12-12 at 11 45 14 Screenshot 2024-12-12 at 11 37 38 Screenshot 2024-12-12 at 12 42 58
Screenshot 2024-12-12 at 11 45 49 Screenshot 2024-12-12 at 11 39 34 Screenshot 2024-12-12 at 12 44 58
Screenshot 2024-12-12 at 11 46 30 Screenshot 2024-12-12 at 11 40 18 image
Screenshot 2024-12-12 at 11 48 07 Screenshot 2024-12-12 at 11 42 03 Screenshot 2024-12-12 at 12 46 14
Screenshot 2024-12-12 at 11 49 05 Screenshot 2024-12-12 at 11 43 40 image

Toasts:

Before After
image image

Other changes:

Before After
Screenshot 2024-12-13 at 17 57 36 Screenshot 2024-12-13 at 18 00 26
Screenshot 2024-12-13 at 18 06 59 Screenshot 2024-12-13 at 18 02 04
Screenshot 2024-12-13 at 18 08 20 Screenshot 2024-12-13 at 18 09 30
Screenshot 2024-12-13 at 18 11 52 Screenshot 2024-12-13 at 18 10 34
Screenshot 2024-12-13 at 18 30 32 Screenshot 2024-12-13 at 18 32 59
Screenshot 2024-12-13 at 18 35 56 Screenshot 2024-12-13 at 18 34 25
image image

@rbrtj rbrtj requested a review from joana-cps December 12, 2024 10:49
@rbrtj rbrtj marked this pull request as ready for review December 17, 2024 09:35
@rbrtj rbrtj requested review from a team as code owners December 17, 2024 09:35
Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

response-ops changes ok

@rbrtj rbrtj self-assigned this Dec 17, 2024
@rbrtj rbrtj added :ml v9.0.0 Team:ML Team label for ML (also use :ml) backport:version Backport to applied version labels v8.18.0 labels Dec 17, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@rbrtj rbrtj changed the title [ML][UX]: Updates to ML embeddable 'Add to' action [ML][UX]: Consistent Layout and UI Enhancements for ML Pages Dec 17, 2024
* Titles for the cases toast messages
*/
export const TITLES = {
CHANGE_POINT_DETECTION: 'Change point detection',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be internationalized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change point chart might be better here. Change point chart added to case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in: #7f408c6

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I'd forgotten that it's possible to multi-select charts to add to a case - can a plural condition be added for Change point chart / Change point charts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added plural for Change point charts.
Also Change point table message when the table view is selected in: #847d00e

pageTitle={
<>
{currentDataView.getName()}
{/* TODO: This management section shouldn't live inside the header */}
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning on doing this in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can tackle it, but it would be nice to find a different place for this menu to reside.
Otherwise, it's being wrapped within an <h1> tag, which doesn't seem right.
If @joana-cps suggests a better location for the button, we can definitely move it outside the header.

Choose a reason for hiding this comment

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

I agree that we need to find a different place for this menu. We can do this in a separate issue.

@@ -321,7 +321,7 @@ export const SeriesControls: FC<PropsWithChildren<SeriesControlsProps>> = ({

return (
<div data-test-subj="mlSingleMetricViewerSeriesControls">
<EuiFlexGroup direction={direction}>
<EuiFlexGroup direction={direction} gutterSize="s">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the detector dropdown be given a minimum width? Also is there any easy way to preserve the vertical alignment of the model bounds, annotations and forecast controls as the width is reduced?

Screenshot 2024-12-17 at 17 07 29

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the detector dropdown to have a min width in: #cd56f03

To preserve vertical alignment, I propose wrapping controls as the width decreases. I don't see any way to keep everything in a single row while maintaining good alignment.

WDYT?

Screen.Recording.2024-12-19.at.11.19.43.mov

Choose a reason for hiding this comment

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

This looks good, I think it is a good solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @joana-cps - this is working well.

Copy link

@joana-cps joana-cps left a comment

Choose a reason for hiding this comment

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

Tested and visually LGTM. Thanks Robert for these improvements!

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

Code LGTM, just added some minor suggestions.

@@ -90,10 +90,6 @@ interface DatePickerWrapperProps {
/**
* Boolean flag to set use of flex group wrapper
*/
flexGroup?: boolean;
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you accidentaly removed the comment for the next prop and not flexGroup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, done in: #d73ab5d

/**
* Titles for the cases toast messages
*/
export const TITLES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to make this a bit more specific like CASES_TOAST_MESSAGES_TITLES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, done in: #7f47fad

/**
* Titles for the cases toast messages
*/
export const TITLES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, suggest to make this const more specific like CASES_TOAST_MESSAGES_TITLES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in: #7f47fad

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.
Just left one suggestion about a plural case for the change point charts. Would be good to get that in too.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
aiops 630 631 +1
ml 2141 2142 +1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
aiops 621.4KB 623.3KB +2.0KB
dataVisualizer 615.4KB 614.9KB -436.0B
ml 4.7MB 4.7MB -3.3KB
transform 476.4KB 476.4KB -8.0B
total -1.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
cases 160.3KB 160.4KB +88.0B
ml 77.8KB 77.8KB +1.0B
total +89.0B
Unknown metric groups

async chunk count

id before after diff
ml 110 111 +1

ESLint disabled line counts

id before after diff
aiops 40 39 -1
ml 557 556 -1
total -2

Total ESLint disabled count

id before after diff
aiops 40 39 -1
ml 560 559 -1
total -2

History

cc @rbrtj

@rbrtj rbrtj merged commit 0cc887b into elastic:main Dec 23, 2024
8 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12465668093

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 203813

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 25, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 203813 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 203813 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 203813 locally

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport missing Added to PRs automatically when the are determined to be missing a backport. backport:version Backport to applied version labels :ml release_note:enhancement Team:ML Team label for ML (also use :ml) v8.18.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants