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.

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

[Alerting] "Create alert" graph visualization design improvements #59399

Merged
merged 10 commits into from
Mar 7, 2020
Merged

[Alerting] "Create alert" graph visualization design improvements #59399

merged 10 commits into from
Mar 7, 2020

Conversation

andreadelrio
Copy link
Contributor

Summary

This PR fixes the issue where context menus were covering the graph visualization in the "create alert" flyout.

It also adds an empty state when the parameters are not enough to generate a graph visualization. This new layout prevents the action picker from jumping (or moving up and down) as the graph visualization changes.

a-loading-alert-new

@mdefazio let me know what you think of the empty state I added and its copy

Fixes #59384

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@andreadelrio andreadelrio requested review from a team as code owners March 5, 2020 04:03
@andreadelrio andreadelrio added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 5, 2020
snide
snide suggested changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on so quick. Some easy comments.

...iggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx Outdated Show resolved Hide resolved
...iggers_actions_ui/public/application/components/builtin_alert_types/threshold/expression.tsx Outdated
<EuiSpacer size="l" />
<div className="alertVisualizationChart">
{canShowVizualization ? (
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely this can be a <Fragment>

x-pack/legacy/plugins/triggers_actions_ui/public/index.scss Outdated Show resolved Hide resolved
x-pack/plugins/triggers_actions_ui/public/application/sections/alert_add/alert_add.scss Outdated Show resolved Hide resolved
pmuellr
pmuellr approved these changes Mar 5, 2020
View reviewed changes
Copy link
Member

@pmuellr pmuellr 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, but I'm not a UI expert

This branch was based on a really old master - about 2 weeks or so. I rebased locally and no conflicts, which slightly surprised me given changes in the plugin. Moving the editors up works well, but there's still some jumpiness, will add a comment in the main PR regarding that.

@pmuellr
Copy link
Member

pmuellr commented Mar 5, 2020

Better!

threshold

There's still some jumpiness, even with the initial "complete the form to show the graph" replacement for the graph at the beginning (not shown above). I'm showing a different interstitial here, the "loading graph data" interstitial. For that one, I'm thinking maybe a separate issue - seems like it would be nice to NOT clear the current graph, but provide some indication that it's "old and going to be replaced". Maybe dim/gray the graph a bit, overlay some text saying "loading new data" or something?

@pmuellr
Copy link
Member

pmuellr commented Mar 5, 2020

ha! I got the same message last night on a GH PR, that I'm seeing here, right now:

This branch has conflicts that must be resolved
Conflicting files
x-pack/plugins/triggers_actions_ui/public/application/sections/alert_add/alert_form.tsx

That file was renamed, but locally git is able to deal with it, without generating a conflict. The GH merge is a little less powerful than local. Hopefully you can rebase this to a recent master and not get any conflicts.

@pmuellr
Copy link
Member

pmuellr commented Mar 5, 2020

You know, looking at this, it's not clear to me we'll know the height of this graph, given that the legend could have LOTS of name/color counts below the graph. Or very few. The number of entries comes from the query result, there should be at most N from the "top N of" selector, but it could be fewer than that. I wonder if we could animate the height change so the stuff below the graph, would at least be less jumpy. Something like that.

YulNaumenko
YulNaumenko approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

@pmuellr pmuellr force-pushed the create-alert-graph branch from 42e5be4 to a274ba8 Compare March 5, 2020 19:12
@mdefazio
Copy link
Contributor

mdefazio commented Mar 6, 2020

Super late on this. Sorry! And I see Dave already mentioned providing the action to the user in the empty state. LGTM!

@andreadelrio andreadelrio requested a review from snide March 6, 2020 20:57
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

snide
snide approved these changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Nice work.

@andreadelrio andreadelrio merged commit 55fa3d6 into elastic:master Mar 7, 2020
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 8, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59399 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59399 or prevent reminders by adding the backport:skip label.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Mar 9, 2020
@gmmorris
Merge branch 'master' into alerting/view-in-app
57d5b4f 
* master: (154 commits)
  Add an optional authentication mode for HTTP resources (elastic#58589)
  Implement embeddable drilldown menu options (elastic#59232)
  [Alerting] "Create alert" graph visualization design improvements (elastic#59399)
  Alerting update route throttle property is missing (elastic#59580)
  [SIEM] Adds 'Load prebuilt rules' Cypress test (elastic#59529)
  Show error if field is not found during filter rendering (elastic#59298)
  Navigate back to discover app during test, because the saved search from the preceding test has major performance problems when used with this test (elastic#59571)
  Check for alert dialog when doing a force logout (elastic#59329)
  ensure fs deletes are not cwd dependent (elastic#59570)
  Empty message for APM service map (elastic#59518)
  [Drilldowns] <ActionWizard/> Component (elastic#59032)
  [Reporting] Improve the page exit error messages (elastic#59351)
  Ensure logged out starting state for tests that need it (elastic#59322)
  Hide input value from kbn-config-schema error messages (elastic#58843)
  [ML] Transforms: Migrate client plugin to NP. (elastic#59443)
  [ML] Disable failing functional tests
  [SIEM] Update Timeline to use the latest euiFlyoutBody style (elastic#59524)
  Temporarily remove the project mappings for PR labels (elastic#59493)
  [Alerting] replace index threshold graph usage of watcher APIs with new API (elastic#59385)
  [ML] Show view series link in anomalies table for machine_learning_user role (elastic#59549)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59399 or prevent reminders by adding the backport:skip label.

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59399 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59399 or prevent reminders by adding the backport:skip label.

@cchaos
Copy link
Contributor

cchaos commented Mar 12, 2020

@andreadelrio This PR needs to be backported to 7.x branch if you need it to get into version 7.7. Let me know if you need a walkthrough, though likely you just need to run node scripts/backport.js

@andreadelrio andreadelrio mentioned this pull request Mar 12, 2020
Closed
andreadelrio added a commit to andreadelrio/kibana that referenced this pull request Mar 13, 2020
@andreadelrio
[Alerting] "Create alert" graph visualization design improvements (el…
488f901 
…astic#59399)
@andreadelrio andreadelrio mentioned this pull request Mar 13, 2020
Merged
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

andreadelrio added a commit that referenced this pull request Mar 13, 2020
@andreadelrio
[Alerting] "Create alert" graph visualization design improvements (#5…
65a38d8 
…9399) (#60078)
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 13, 2020
@andreadelrio andreadelrio added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YulNaumenko YulNaumenko YulNaumenko approved these changes

@snide snide snide approved these changes

@pmuellr pmuellr pmuellr approved these changes

Assignees
No one assigned
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Alerting] index threshold graph obscured by expression editor
8 participants
@andreadelrio @pmuellr @mdefazio @kibanamachine @cchaos @snide @YulNaumenko @thompsongl