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] Standardize Add embeddable flow from the Anomaly Explorer page #123199

Merged
merged 7 commits into from
Jan 19, 2022

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Jan 18, 2022

Summary

Standardizes the workflow for adding an embeddable swim lane or anomaly charts from the Anomaly Explorer to Dashboard so that it is aligned with that used in other places in Kibana. Users are now always redirected to Dashboard app after adding the embeddable so that they can modify and save their changes inside Dashboard.

Resolves #122300 and #122271

Updates the user flow of adding ML embeddables to dashboards from the Anomaly Explorer page by utilizing the embeddable transfer state service. Due to the lack of alternative solutions, we used to manipulate dashboards saved objects directly. Now the user is redirected to the dashboard in edit mode and should save changes explicitly.

Important differences with the previous version:

  • It's not possible anymore to quickly add embeddable to a dashboard in the background. Also manipulating multiple dashboards isn't allowed.
  • It's not possible anymore to add multiple embeddables at the time, i.e. the user must select Overall or View By swim lane, but not both.
  • Each row in the "Add to dashboard" dialog got an action to navigate to the dashboard in edit mode with pending changes that contain the embeddable config

Jan-18-2022 10-41-36

Checklist

@darnautov darnautov added :ml Feature:Anomaly Detection ML anomaly detection release_note:skip Skip the PR/issue when compiling release notes v8.1.0 Team:ML Team label for ML (also use :ml) labels Jan 18, 2022
@darnautov darnautov requested a review from a team as a code owner January 18, 2022 09:57
@darnautov darnautov self-assigned this Jan 18, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson peteharverson added release_note:feature Makes this part of the condensed release notes release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes release_note:feature Makes this part of the condensed release notes labels Jan 18, 2022
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 and LGTM

description: i18n.translate('xpack.ml.explorer.dashboardsTable.editActionName', {
defaultMessage: 'Add to dashboard',
}),
icon: 'documentEdit',
Copy link
Contributor

Choose a reason for hiding this comment

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

This icon implies more of an 'edit' action to me than 'add', but can't find anything better! You are redirected to Dashboard to allow more 'editing', so maybe it's ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

listAdd icon (take a look in https://elastic.github.io/eui/#/display/icons) might also work 🤔

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
ml 3.5MB 3.5MB -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
ml 38.5KB 38.5KB +22.0B
Unknown metric groups

References to deprecated APIs

id before after diff
ml 63 67 +4

History

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

cc @darnautov

Copy link
Contributor

@ThomThomson ThomThomson left a comment

Choose a reason for hiding this comment

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

Tested this locally on chrome, everything works as expected! The user is redirected to the dashboard, and the swimlane embeddable works as expected.

I also looked through the diff, and it's great to see the last usage of the URL generator getting removed, very exciting!

I am approving, but I think there are many UX improvements to be made in the future to better align this with how other Kibana apps add their embeddables to a dashboard.

  • Redirecting to a dashboard is a pretty major action, so using such a small action button per-row seems jarring from a UX POV. Additionally, there is no description stating that you will be redirected.
  • There is no way to add the swimlane embeddable directly to a new dashboard
  • The inline dashboard selection table causes layout shifting in the modal depending on how many dashboards are available.

If we eventually re-think the UX of this window, we should look at the way Visualize / Lens accomplishes this:

Screen Shot 2022-01-18 at 12 03 21 PM

We would need to remove the None / add to Library radio buttons, but the rest of the UI could be retained.

@darnautov darnautov merged commit 0e112df into elastic:main Jan 19, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jan 19, 2022
@darnautov darnautov deleted the ml-fix-add-embeddable-to-dashbaord branch January 19, 2022 09:12
@darnautov
Copy link
Contributor Author

Thanks for the review @ThomThomson! Agree, we need to rethink the UX here because originally it was possible to change multiple dashboards at once. Visualize / Lens approach seems sensible! Merged it to remove the blocker, but we'll get back to it.

@ThomThomson
Copy link
Contributor

Amazing, thanks again for doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Anomaly Detection ML anomaly detection :ml release_note:enhancement Team:ML Team label for ML (also use :ml) v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ML][Dashboard] Standardize Add Embeddable from Anomaly Explorer Page
6 participants