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

[SIEM] Detection Engine Create Rule Design Review #1 #54442

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Jan 10, 2020

Summary

All form fields in each step panel should be left-aligned with the beginning of the step header text, as indicated in the wireframes and mockups. Currently, they are left-aligned with the numbered/checked circle

Screenshot 2020-01-10 at 13 42 53
Screenshot 2020-01-10 at 13 43 06
Screenshot 2020-01-10 at 13 43 15

The “Index patterns” field button to “Reset to default index patterns” is missing the prefixed refresh icon indicated in the mockups
Screenshot 2020-01-10 at 13 44 29

In the “Severity” field, it appears as though the EuiHealth component within the field is off-center vertically
Screenshot 2020-01-10 at 13 45 38

Can the “False positives” field label be renamed to “False positive examples” and the subsequent “Add false positive” button text be changed to “Add false positive example”?
Screenshot 2020-01-10 at 13 48 23

Alignment should again be left-aligned with step header text, not with the checked circle
Screenshot 2020-01-10 at 13 47 45

The “Description” field appears in a disabled textarea, which is not semantically correct and doesn't match the designs.
The “Severity” field’s value appears to be lower case, when it should be sentence case (i.e. “low” vs “Low”).
The “Risk score” field is missing from the completed step summary.
Can we change the “False positives” field label to be “False positive examples”?
Screenshot 2020-01-10 at 13 49 28

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

This is looking great! Thanks so much for the fast fixes, @patrykkopycinski! Leaving a few small comments below.

  • Thanks for correcting the alignment/indentation of the form fields. Unfortunately, it looks like this change also indented in the hr at the bottom of each step panel (above the "Continue" buttons). Is there a way that the bottom hr elements can not be indented and instead match the alignment and width of the top hrs, as shown in the designs?

  • It look like with these changes, the completed step summaries are now laid out in a single column. Is there any way we can restore the completed step summaries to being in a two column layout (even if "Description" field can’t go the full width to match the designs)?

patrykkopycinski and others added 5 commits January 11, 2020 13:53
…tion-engine-design-review-1

# Conflicts:
#	x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/all/columns.tsx
#	x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/create/index.tsx
@patrykkopycinski
Copy link
Contributor Author

Focusing the “Custom query” field brings up the autocomplete menu. The autocomplete menu appears to be very short vertically and also increases the height of the “Define rule” step when opened.

Screenshot 2020-01-13 at 11 51 25

Thanks for correcting the alignment/indentation of the form fields. Unfortunately, it looks like this change also indented in the hr at the bottom of each step panel (above the "Continue" buttons). Is there a way that the bottom hr elements can not be indented and instead match the alignment and width of the top hrs, as shown in the designs?

Screenshot 2020-01-13 at 11 52 51

It look like with these changes, the completed step summaries are now laid out in a single column. Is there any way we can restore the completed step summaries to being in a two column layout (even if "Description" field can’t go the full width to match the designs)?

Screenshot 2020-01-13 at 11 53 22

The spacing in between each step panel appears to be smaller than indicated in the wireframes/mockups. Can we change to 24px space in between each step panel to mimic the design?

Screenshot 2020-01-13 at 11 56 56

The EuiHorizontalRule under each step panel header is a different/incorrect margin size versus the one used in the step panel footer. Can the one under the header be changed to margin="m"?

When altering the “Index patterns” field, the alignment of the label changes and shifts up. Doing so misaligns the label, as it doesn’t share the same baseline as the “Reset to default index patterns” text. (P) use IconButton

S8mNTOQyg6

All subordinate actions (“Add reference URL”, “Add false positive example”, “Add MITRE ATT&CK threat”) appear to be larger than designed in both icon and font size. Can we change this to use a 12x12 icon and 12px font size?

Screenshot 2020-01-13 at 12 00 50

Both time unit select elements for “Rule run interval & look-back` and “Additional look-back” appear as if nested inside and overflowing out the bottom of the preceding time integer input. Can we clean this up to make them appear adjacent rather than nested?

The helper text below the “Additional look-back” field appears to be breaking a line at an arbitrary point. Can this just be allowed to stretch the width of the given space? Or if not, perhaps confined to the width of the form fields? (X maybe)

Screenshot 2020-01-13 at 12 02 30

The “MITRE ATT&CK” field’s tactic value is the wrong font size and weight. (P)
The “MITRE ATT&CK” field’s technique values are missing the prefixed ∟ icon. (P)

Screenshot 2020-01-13 at 12 17 00

Screenshot 2020-01-13 at 12 18 15

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

This looks great, @patrykkopycinski. I only noticed one super small thing, but marking this as approved for after. Comment below. Thanks!

optional-color

It appears that the "Optional" text for "Additional look-back" field is the incorrect color (should match the rest). Also when the field is focused, it turns blue (like the label). That's not a pattern that happens in the fields above. If we can lose the focus color on the "Optional" text here, that would be great.

@patrykkopycinski
Copy link
Contributor Author

Thank you @MichaelMarcialis 👍
Screenshot 2020-01-13 at 18 11 19

@patrykkopycinski
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 14, 2020
* upstream/master: (26 commits)
  Take page offset into account too (elastic#54567)
  [APM] Support error.{log,exception}.stacktrace.classname (elastic#54577)
  Np migration tsvb route validation (elastic#51850)
  [ML] MML calculator enhancements for multi-metric job wizard  (elastic#54573)
  [SIEM] Fix Inspect query 'request timestamp' value changes when curso… (elastic#54223)
  Fix chromeless NP apps not using full page width (elastic#54550)
  Remove extraneous public import to prevent failing Kibana startup (elastic#54676)
  [Uptime] Temporarily skip flakey tests (elastic#54675)
  Skip failing uptime tests
  Create UI for alerting and actions plugin (elastic#48959)
  [dev/build/sass] build stylesheets for disabled plugins too (elastic#54654)
  [SIEM] Use bulk actions API when updating or deleting rules (elastic#54521)
  Support "Deprecated" label in advanced settings (elastic#54539)
  [Maps] add text halo color and width style properties (elastic#53827)
  Service Map Data API at Runtime (elastic#54027)
  [SIEM] Detection Engine Create Rule Design Review #1 (elastic#54442)
  Skip flaky test
  [Canvas] Enable Embeddable maps (elastic#53971)
  [SIEM][Detection Engine] Increases the number or rules you can view on a single page (elastic#54628)
  uiSettings - use validation field for image field maxSize (elastic#54522)
  ...
jkelastic pushed a commit to jkelastic/kibana that referenced this pull request Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:SIEM v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants