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

[Alerting] "Create alert" and alert list design improvements #63515

Merged
merged 19 commits into from
Apr 17, 2020

Conversation

andreadelrio
Copy link
Contributor

Summary

"Create alert" changes

  • Adjust spacing around visualization chart area.
  • Adjust typography in the Actions section. Made the Actions header visible after the user has picked an action.
  • Adjusted buttons size
  • Adjusted placement of Add action button and added a horizontal rule above it
  • Adjusted layout of KeyPadMenuItems showing actions to avoid having extra white space on the right.
  • Add external icon to "Get more actions" link

[Create Alert] Actions section

Now
actions_new
Before
actions_old

"Alert list" changes

  • Improved styling in bulk actions menu
  • Improved styling in collapsed item actions menu, including change to compressed switches.

Now
image
image

Before
image
image

@andreadelrio andreadelrio added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Apr 14, 2020
@andreadelrio andreadelrio requested review from a team as code owners April 14, 2020 19:33
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

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

@andreadelrio andreadelrio added the release_note:skip Skip the PR/issue when compiling release notes label Apr 14, 2020
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.

Just a comment when looking at one of your screens. Didn't run this locally yet.

image

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Thank you for doing this PR for us! LGTM, I just had a few notes.

@andreadelrio
Copy link
Contributor Author

Just a comment when looking at one of your screens. Didn't run this locally yet.

image

@snide I tried many routes to fix the alignment of the trash icon and the delete label, including EuiFlexGroup and EuiButtonEmpty. In all cases something felt off. The only solution I found was adding some padding-top to the label. It’s not the cleanest route but I just couldn’t get this to feel right using flex alignment properties, I think that might be because of the icon we’re using.

image

^ This is what it looks like right now. I worked on the copy with Gail and DeFazio. I did not use EuiContextMenuItem for Disable and Mute because I didn't want the help text to be clickable or to have an underline whenever the user hovers over the switches. Result:

new_actions

Also a note for other reviewers that we've changed "Enable" to "Disable" so that we have all of them be "negative" actions.

@mdefazio
Copy link
Contributor

These look great. The popover for the row actions is much better. We can maybe try and align the label (Disable/Mute) with the definition instead of with the switch.

@andreadelrio andreadelrio requested a review from snide April 16, 2020 01:06
Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Tested the changes locally and still LGTM 👍

@gchaps
Copy link
Contributor

gchaps commented Apr 16, 2020

@andreadelrio Please add a period at the end of each sentence.

When disabled, the alert is not checked.
When muted, the alert is checked, but no action is performed.

@mikecote mikecote added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 16, 2020
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@andreadelrio andreadelrio merged commit c13a026 into elastic:master Apr 17, 2020
andreadelrio added a commit to andreadelrio/kibana that referenced this pull request Apr 17, 2020
…#63515)

* added header to actions section

* adjusted some spacing in Create Alert

* lighter text in Actions section headings

* fixed bulk actions dropdown

* improve alert collapsed item actions

* improve dropdown and adjust some buttos

* adjust font size of steps to match hierarchy

* need to check master

* improve collapsed actions menu

* added periods to help texts

* going back to EuiButtonEmpty to be able to use isLoading

* fix prop

* remove Fragment

* remove translations we're not using

* Fix functional tests

Co-authored-by: Mike Cote <mikecote@users.noreply.github.com>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 20, 2020
…bana into ingest-node-pipelines/privileges

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (126 commits)
  [SEARCH] Cleanup fetch soon (elastic#63320)
  skip flaky suite (elastic#58692)
  [Uptime] Refresh index and also show more info to user regardi… (elastic#62606)
  [Drilldowns] Fix back button by removing panels from url in dashboard in view mode (elastic#62415)
  [platform] serve plugins from /bundles/plugin:${id}
  [Alerting] Documentation for how to pre-configure connectors. (elastic#63807)
  skip flaky suite (elastic#63621)
  Revert "skip flaky suite (elastic#63747)"
  skip flaky suite (elastic#63747)
  [SIEM][Detections Engine] - Update rule.lists to be rule.exceptions_list (elastic#63717)
  [SIEM] Flaky test fix: Bump find_statuses timeout (elastic#63900)
  [Uptime] Add cert API request and runtime type checking (elastic#63062)
  [Lens] Allow table to scroll horizontally (elastic#63805)
  [Metrics UI] Allow users to create alerts from the central Alerts UI (elastic#63803)
  Migrate legacy maps licensing (x-pack/tilemap) to NP (elastic#63539)
  [Alerting] "Create alert" and alert list design improvements (elastic#63515)
  [Lens] Fix existence for dotted paths in _source (elastic#63752)
  Example plugins in X-Pack (elastic#63823)
  [ML] Migrate Mocha unit tests to Jest: migrate job utils and query utils tests (elastic#63775)
  Endpoint: middleware receive immutable versions of state and actions (elastic#63802)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 20, 2020
…bana into pipeline-editor-part-mvp-2

* 'feature/ingest-node-pipelines' of github.com:elastic/kibana: (127 commits)
  [Ingest pipelines] Polish details panel and empty list (elastic#63926)
  [SEARCH] Cleanup fetch soon (elastic#63320)
  skip flaky suite (elastic#58692)
  [Uptime] Refresh index and also show more info to user regardi… (elastic#62606)
  [Drilldowns] Fix back button by removing panels from url in dashboard in view mode (elastic#62415)
  [platform] serve plugins from /bundles/plugin:${id}
  [Alerting] Documentation for how to pre-configure connectors. (elastic#63807)
  skip flaky suite (elastic#63621)
  Revert "skip flaky suite (elastic#63747)"
  skip flaky suite (elastic#63747)
  [SIEM][Detections Engine] - Update rule.lists to be rule.exceptions_list (elastic#63717)
  [SIEM] Flaky test fix: Bump find_statuses timeout (elastic#63900)
  [Uptime] Add cert API request and runtime type checking (elastic#63062)
  [Lens] Allow table to scroll horizontally (elastic#63805)
  [Metrics UI] Allow users to create alerts from the central Alerts UI (elastic#63803)
  Migrate legacy maps licensing (x-pack/tilemap) to NP (elastic#63539)
  [Alerting] "Create alert" and alert list design improvements (elastic#63515)
  [Lens] Fix existence for dotted paths in _source (elastic#63752)
  Example plugins in X-Pack (elastic#63823)
  [ML] Migrate Mocha unit tests to Jest: migrate job utils and query utils tests (elastic#63775)
  ...
andreadelrio added a commit that referenced this pull request Apr 20, 2020
…#63899)

* added header to actions section

* adjusted some spacing in Create Alert

* lighter text in Actions section headings

* fixed bulk actions dropdown

* improve alert collapsed item actions

* improve dropdown and adjust some buttos

* adjust font size of steps to match hierarchy

* need to check master

* improve collapsed actions menu

* added periods to help texts

* going back to EuiButtonEmpty to be able to use isLoading

* fix prop

* remove Fragment

* remove translations we're not using

* Fix functional tests

Co-authored-by: Mike Cote <mikecote@users.noreply.github.com>

Co-authored-by: Mike Cote <mikecote@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants