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 Icons #2704

Merged
merged 12 commits into from
Jan 4, 2020

Conversation

MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Dec 19, 2019

Summary

Add icons to EUI to support the needs of the upcoming detection engine UI, including the following additions:

  • aggregate
  • pageSelect
  • pagesSelect
  • securitySignal
  • securitySignalDetected
  • securitySignalResolved
  • timeline

Issue

image

Checklist

  • Check against all themes for compatability in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
    - [ ] Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
    - [ ] Checked for breaking changes and labeled appropriately
    - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos
Copy link
Contributor

cchaos commented Dec 19, 2019

Discussion question: Are these glyphs something to be considered as custom for the SIEM application? My consideration stems around what happens when other applications start using these icons. Does it start altering the meaning/recognition of the icon within the SIEM app?

Mainly I'm worried about the signal and aggregate icons. I think the checkbox and timeline icons are generic enough that they could be used outside of the context of SIEM.

@MichaelMarcialis MichaelMarcialis marked this pull request as ready for review December 19, 2019 22:26
@ryankeairns
Copy link
Contributor

I agree with Caroline, the consideration with regards to these getting used elsewhere and losing meaning feels most important. Related and somewhat contrary to that, would these be re-usable in other security apps like Endpoint? That may strengthen the case for adding them to EUI.

If these were to be added, we could also consider prefixing them with something like securitySignal as we have with the logstash* set. That prefix makes people think twice before using them in another fashion.

@MichaelMarcialis
Copy link
Contributor Author

All good points, @cchaos and @ryankeairns. I think signal, signalDetected, signalResolved and timeline are all currently specific to the security apps in Kibana (SIEM and Endpoint). There is potential for that reach to be widened in the future, but this is the scope at present. I'd be happy to prefix these with security if that would be more desirable.

I think aggregate, checkbox and checkboxMultiple are generic enough that they wouldn't require such a prefix. Thoughts?

@cchaos
Copy link
Contributor

cchaos commented Dec 20, 2019

Lets just prefix the signal ones. It seems there may be generic uses for aggregate and timeline.

This is mainly a reminder because I know these icons can be tricky to get completely pixel perfect because of the complexity of them. However, your aggregate and timeline ones will probably suffer quite a bit when viewed on non-retina screens because of the half pixel points.

Screen Shot 2019-12-20 at 10 18 42 AMScreen Shot 2019-12-20 at 10 18 56 AM

Knowing it's not ideal to have the icon be off-centered in the 16x16 frame, this is usually how we attain pixel alignment. I would just be sure that you check how these icons look on non-retina screens.

@MichaelMarcialis
Copy link
Contributor Author

@cchaos: Sorry for the delayed response. Got pulled away before the holiday vacations started.

Thanks for the feedback. Sounds good. I've updated this PR to prefix the security-centric icons and also better align the aggregate and timeline icons to the pixel grid.

@cchaos
Copy link
Contributor

cchaos commented Jan 2, 2020

No worries @MichaelMarcialis ! Can you also update the screenshots in the summary of the PR?

Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

To pull this a bit more into the 'family' 👩‍👧‍👧 , can we replace the checkmark and exclamation point with those used in other icons? For example, the checkInCircleFilled could be used on the three glyphs. Likewise, we have an exclamation point in the warning icon that could be used in the filled circle shape too (just ditch the triangle and use the exclamation point).

Screenshot 2020-01-03 09 36 48

Screenshot 2020-01-03 09 44 08

Unrelated, I think you should be safe to remove the clip-rule="evenodd" and fill-rule="evenodd" from your SVGs.

@MichaelMarcialis
Copy link
Contributor Author

To pull this a bit more into the 'family' 👩‍👧‍👧 , can we replace the checkmark and exclamation point with those used in other icons? For example, the checkInCircleFilled could be used on the three glyphs. Likewise, we have an exclamation point in the warning icon that could be used in the filled circle shape too (just ditch the triangle and use the exclamation point).

Screenshot 2020-01-03 09 36 48 Screenshot 2020-01-03 09 44 08

Unrelated, I think you should be safe to remove the clip-rule="evenodd" and fill-rule="evenodd" from your SVGs.

Hey, @ryankeairns. Thanks for the comments!

Regarding the 'family' comment, I thought the same initially. I ended up not taking that direction as I feared that copying and scaling down the checkInCircleFilled icon would yield a check that wouldn't sit properly on the pixel grid and would also create a stroke that was less than 1px. Additionally, I noticed that the exclamation point currently in the alert icon doesn't sit perfectly on the pixel grid, so I opted to create a simple one that did so (to ensure it would be easily visible at the small size in which I'm using it). Is my thinking flawed there? If so, let me know and I'll be happy to do as you suggest.

As for removing clip-rule and fill-rule, it looks like I can safely remove clip-rule, but removing fill-rule appears to have adverse affects. Thoughts there?

@ryankeairns
Copy link
Contributor

ryankeairns commented Jan 3, 2020

That makes sense regarding scaling it down and becoming too thin. Initially, the checkmark struck me as being a different style, but it is the same weight and has rounded ends/corners... it simply looks different at this small size but oh well :) I don't see a way around that since it needs to fit in this circle shape, however I do think we should center the checkmark and live with it not lying perfectly on the grid. The off-centered checkmark is less desirable, in my opinion.

I played around with this a little bit and then ran the Figma SVG through Sketch (imported, exported) as it does a better job cleaning up the SVG (i.e. it removes the fill-rule bit).

Centering the current checkmark you have seems the best option... squared and scaled shown below for comparison:

32px

PR version | centered | centered with square ends | scaled down checkInFilledCircle

Screenshot 2020-01-03 12 13 18

16px

PR version | centered | centered with square ends | scaled down checkInFilledCircle

Screenshot 2020-01-03 12 13 36

@MichaelMarcialis
Copy link
Contributor Author

@ryankeairns, thanks for doing that. I played around with the check/exclamation circles a bit more. I increased the circle diameters by 1px and altered the exclamation's shape to account for the new alignment. This allows for both check and exclamation to be visually centered, be mostly on the pixel grid, and appears to be the most legible of all options. Updated screenshot is at the top. Thoughts?

Also, I used a Figma plugin called Advanced SVG Export that appears to have done a better job at optimizing the SVGs than standard Figma exporting.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I think the newest versions make the exclamation more noticeable and distinguishable from the checks. 👍 LGTM!

@ryankeairns ryankeairns self-requested a review January 3, 2020 22:36
Copy link
Contributor

@ryankeairns ryankeairns left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the further tweaks, much appreciated. Keeping an icon set in order is like tending a garden :)

@MichaelMarcialis MichaelMarcialis merged commit 341e8f8 into elastic:master Jan 4, 2020
@MichaelMarcialis MichaelMarcialis deleted the siem-new-icons branch January 4, 2020 05:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants