Skip to content

[Security Solution] Changes rules table tag display#77102

Merged
dplumlee merged 7 commits intoelastic:masterfrom
dplumlee:rules-table-tag-display
Oct 2, 2020
Merged

[Security Solution] Changes rules table tag display#77102
dplumlee merged 7 commits intoelastic:masterfrom
dplumlee:rules-table-tag-display

Conversation

@dplumlee
Copy link
Copy Markdown
Contributor

@dplumlee dplumlee commented Sep 9, 2020 •
edited
Loading

Summary

Changes rules table tag display to show 3 initial tags and then display the full list in a scrollable popover

Screenshots

Limits width of tag popover (has title hover)
Screen Shot 2020-10-01 at 3 31 07 PM

Displays a popover button if the tag amount is over 3
Screen Shot 2020-10-01 at 3 30 31 PM

Popover limits tag length and is scrollable
Screen Shot 2020-10-01 at 3 30 41 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Sep 9, 2020
@dplumlee dplumlee self-assigned this Sep 9, 2020
@dplumlee dplumlee added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Sep 9, 2020
@dplumlee dplumlee marked this pull request as ready for review September 9, 2020 18:59
@dplumlee dplumlee requested review from a team as code owners September 9, 2020 18:59
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 9, 2020

Pinging @elastic/siem (Team:SIEM)

@dplumlee dplumlee force-pushed the rules-table-tag-display branch from cac806b to 40f31ac Compare September 10, 2020 17:20
yctercero
yctercero reviewed Sep 10, 2020
View reviewed changes
x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/columns.tsx Outdated
Copy link
Copy Markdown
Contributor

@yctercero yctercero Sep 10, 2020

Choose a reason for hiding this comment

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

Could we add a quick test in x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/columns.test.tsx that tests that all tags are rendered/visible?

yctercero
yctercero approved these changes Sep 10, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

LGTM! Just one small comment on adding a quick unit test if you can.

@spong
Copy link
Copy Markdown
Member

spong commented Sep 16, 2020

@elasticmachine merge upstream

@spong
Copy link
Copy Markdown
Member

spong commented Sep 18, 2020 •
edited
Loading

For this I think we'll still want some sort of overflow option as things can get quite hairy with vertical space when there's quite a few tags (which sounds like the protections team will be adding plenty of in the coming release 😅 ):

There's two bounds to cover here:

  1. When a tag is reeeeeeally long
  2. When there are a lot of tags

And looks like there's five touch points for these bounds:

  1. Rule Creation / Edit Rules
  2. Rule Details
  3. Rules Table
  4. Rules Table Tag Selection
  5. Detection Alerts Table

Going over these:

  1. Rules Creation looks good on both fronts! 👍 Long tags are truncated per line, and a fair number of tags display without too much vertical space (can always set a max height and enable scroll if this becomes an issue)

  1. Rule Details fairs pretty well too, but we're working with quite a bit more space here. Doesn't look like reeeeeeally long tags truncate, but this should be a corner case.

  1. Rules Table, as above, is probably where this will be a pain point for our users. Reeeeeeally long tags truncate per line and have hover title hover text 👍, so that's all good, but many tags will keep expanding the row height. For this, I think the easiest and nicest solution for our users would be to cap the row height, and then expose the overflow tags in a scrollable popover -- could just reuse the tag selection component w/ selection disabled.

  2. Rules Table Tag Selection looks good, but gets a bit unwieldy when dealing with reeeeeeally long tags. As above, this is a corner case, but it'd be an easy fix to set a max-width on the popover and ensure there's either horizontal scroll or at least title hover text.

image

  1. Detection Alerts Table looks to be having the same issue as Rule Name ([SIEM] Signal's rule name is not showing properly on signals table #65764) and just shows the empty value --. On expansion of the alert, the tags display fairly well, one per line, with no truncation, but as mentioned above this should be a corner case.

All that said, no need to fix all these in this much-appreciated enhancement! 😅 I do think we should address item 3. though as with the influx of tags from protections this release the row-heights are going to be all over the place if we don't set a max, so this would probably be the most impactful to users.

@dplumlee dplumlee marked this pull request as draft September 21, 2020 19:40
@dplumlee dplumlee force-pushed the rules-table-tag-display branch from 50f3060 to bb163a9 Compare October 1, 2020 21:35
@dplumlee dplumlee marked this pull request as ready for review October 1, 2020 21:40
@dplumlee dplumlee requested review from spong and yctercero October 1, 2020 21:41
@dplumlee
Copy link
Copy Markdown
Contributor Author

dplumlee commented Oct 1, 2020

@spong @yctercero I refactored a lot of this including the overall design to address most of the issues garrett brought up so thought i'd re-request reviews

spong
spong approved these changes Oct 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@spong spong 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 cleaning up the UX so things stay manageable on the All Rules table @dplumlee! 🙂

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Oct 2, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
securitySolution 1978 1979 +1

async chunks size

id before after diff
securitySolution 10.3MB 10.3MB +5.3KB

History

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

@dplumlee dplumlee merged commit bd80d3c into elastic:master Oct 2, 2020
@dplumlee dplumlee deleted the rules-table-tag-display branch October 2, 2020 22:19
@dplumlee dplumlee mentioned this pull request Oct 2, 2020
Merged
dplumlee added a commit that referenced this pull request Oct 3, 2020
@dplumlee
[Security Solution] Changes rules table tag display (#77102) (#79380)
33d3784
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 5, 2020
@gmmorris
Merge branch 'master' into actions/decouple-actions-from-alertid
d99fe4b 
* master: (128 commits)
  add core-js production dependency (elastic#79395)
  Add support for sharing saved objects to all spaces (elastic#76132)
  [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038)
  load js-yaml lazily (elastic#79092)
  skip flaky suite (elastic#77278)
  Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341)
  [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988)
  [Security Solutions][Detection Engine] Updates the edit rules page to:wq! only have what is selected for editing (elastic#79233)
  Cleanup yarn.lock from duplicates (elastic#66617)
  [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052)
  [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291)
  [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358)
  [babel/register] remove from build (take 2) (elastic#79379)
  [Security Solution] Changes rules table tag display (elastic#77102)
  define integrationTestRoot in config file and use to define screensho… (elastic#79247)
  Revert "[babel/register] remove from build (elastic#79176)"
  skip flaky suite (elastic#75241)
  [Uptime] Synthetics UI (elastic#77960)
  [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812)
  [babel/register] remove from build (elastic#79176)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 5, 2020
@gmmorris
Merge branch 'master' into task-manager/health
7b14dff 
* master: (288 commits)
  add core-js production dependency (elastic#79395)
  Add support for sharing saved objects to all spaces (elastic#76132)
  [Alerting UI] Display a banner to users when some alerts have failures, added alert statuses column and filters (elastic#79038)
  load js-yaml lazily (elastic#79092)
  skip flaky suite (elastic#77278)
  Fix agentPolicyUpdateEventHandler() to use app context soClient for creation of actions (elastic#79341)
  [Security Solution] Untitled Timeline created when first action is to add note (elastic#78988)
  [Security Solutions][Detection Engine] Updates the edit rules page to only have what is selected for editing (elastic#79233)
  Cleanup yarn.lock from duplicates (elastic#66617)
  [kbn/optimizer] implement more efficient auto transpilation for node (elastic#79052)
  [Ingest Manager] Rename Fleet setup and requirement, Fleet => Central… (elastic#79291)
  [core/server/plugins] don't run discovery in dev server parent process (take 2) (elastic#79358)
  [babel/register] remove from build (take 2) (elastic#79379)
  [Security Solution] Changes rules table tag display (elastic#77102)
  define integrationTestRoot in config file and use to define screensho… (elastic#79247)
  Revert "[babel/register] remove from build (elastic#79176)"
  skip flaky suite (elastic#75241)
  [Uptime] Synthetics UI (elastic#77960)
  [Security Solution] [Detections] Only display actions options if user has "read" privileges (elastic#78812)
  [babel/register] remove from build (elastic#79176)
  ...
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Sep 23, 2021

Pinging @elastic/security-solution (Team: SecuritySolution)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@spong spong spong approved these changes

@yctercero yctercero Awaiting requested review from yctercero

Assignees

@dplumlee dplumlee

Labels

release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dplumlee @elasticmachine @spong @kibanamachine @yctercero @MindyRS