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

[Security Solution][Alerts] Make rule preview flyout resizable #147351

Merged
merged 18 commits into from
Jan 18, 2023

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Dec 12, 2022

Summary

This PR makes Rule Preview component resizable.

Screen.Recording.2023-01-10.at.18.18.20.mov

Closes #146847

@e40pud e40pud added release_note:enhancement Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Alerts Security Detection Alerts Area Team ci:cloud-deploy Create or update a Cloud deployment labels Dec 12, 2022
@e40pud e40pud self-assigned this Dec 12, 2022
@e40pud
Copy link
Contributor Author

e40pud commented Dec 14, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 19, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 20, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 23, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 27, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 28, 2022

@ARWNightingale could you check these changes from UI/UX perspective please? We decided to move from having Rule Preview in EuiFlyout to EuiResizableContainer. There were a few issues with flyout component, so we decided to replace it. You can see changes in video recording in description and if you'd like to play with the current state then you can use cloud deployment (let me know if you need help with that).

@e40pud
Copy link
Contributor Author

e40pud commented Dec 28, 2022

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Dec 30, 2022

@elasticmachine merge upstream

@e40pud e40pud marked this pull request as ready for review December 30, 2022 10:12
@e40pud e40pud requested review from a team as code owners December 30, 2022 10:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@ARWNightingale
Copy link

I am just checking with the EUI team in regards to another way we can action the interaction. For me it seems very hidden that the feature is there. Maybe we can just use another button to trigger the container.

@ARWNightingale
Copy link

@e40pud I have spoken to the EUI team and we can trigger the container using toggles like this. I suggest we add a toggle that replaces the existing button (pre-exiting rule preview button that was sitting on the top right). This would reduce learning new behaviour for our users. we can still keep the collapse and expand options too.

https://elastic.github.io/eui/#/layout/resizable-container#collapsible-panels-with-external-control

Copy link
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.

Checked out, tested locally, code reviewed and overall functionality LGTM! 🎉 👍 Keyboard navigation works as expected (minus this other bug I found), and reflow perf seems reasonable given the content on the screen.

As @ARWNightingale commented, I agree it would be a good idea to bring back the preview button and have its action be the external control for showing/hiding preview container.

Additionally, a few small UI nits:

  1. The min-size on the preview container allows for cutoff of the datepicker action button. Consider increasing the min-width ever so slightly. Or perhaps this is an overflow issue with the timepicker component?

  1. If min-height is less than ~870px, the resize icon isn't displayed (since it's mid-way of total page height). Not a big deal once we move back to the dedicated button, and really only an issue when needing to collapse since the bar itself can be used to expand (but only the icon to collapse).

  1. Would be nice if we could have the resize-bar extend to the edge of the page on the right (and perhaps the top/bottom of the page as well), but I understand we're working within the SecurityPageWrapper component which is adding this 24px padding. Just looks a little weird floating there (IMO) when comparing to the EUI examples. Not a huge deal either way.

edit: maybe this isn't so bad after all, as it prevents mis-clicks on the scroll bar, and won't be the only CTA for opening the panel once we add the button back. Please ignore this one... 😅

@e40pud
Copy link
Contributor Author

e40pud commented Jan 4, 2023

@elasticmachine merge upstream

Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

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

@e40pud, thanks for this improvement. It looks good code wise and I have few questions regarding user experience

  1. Agree on returning back Preview button, it might not be easy for users to open preview back, once it's closed without Preview button.
  2. Maybe we can add a tooltip when preview section is closed when you navigate over closed resize bar? I'm not sure 100% sure it a something worth to do, but we could consider it
Screen.Recording.2023-01-05.at.11.35.38.mov
  1. On small resolution new implementation become difficult to use
    Here is old implementation with flyout:

Screenshot 2023-01-05 at 11 29 15

Here is a new one:
Screenshot 2023-01-05 at 11 29 51

Maybe stacking it on each other on small resolution can be viable solution? It seems could be implemented easier than introducing overlay on that resolution. Although, if it possible, I would prefer overlay on small screens and resizable implementation on bigger

@e40pud
Copy link
Contributor Author

e40pud commented Jan 10, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 10, 2023

@spong @vitaliidm As Alex suggested, I returned the Rule Preview button which fixed most of the concerns. We might need to fix the issue with extremely small screens, though I feel like we could do that in a separate PR. @vitaliidm we cannot really switch from resizable component to overlayed flyout component as it will introduce bigger issues with state being vanished. We already have that issue with the current implementation when user decreases the size of the window to a very narrow one. In that case flyout changes type from pushed to overlayed automatically and removes all the Rule Preview content.

@e40pud
Copy link
Contributor Author

e40pud commented Jan 12, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 12, 2023

@elasticmachine merge upstream

@vitaliidm
Copy link
Contributor

@e40pud

I've noticed Preview Button is absent on Rule Edit page

Screenshot 2023-01-13 at 10 25 30

@vitaliidm we cannot really switch from resizable component to overlayed flyout component as it will introduce bigger issues with state being vanished. We already have that issue with the current implementation when user decreases the size of the window to a very narrow one. In that case flyout changes type from pushed to overlayed automatically and removes all the Rule Preview content.

I've been thinking of using https://elastic.github.io/eui/#/layout/flex for this purpose, but I don't know how that would work with resizable container. Can you please create a ticket to address small screen issue?

@e40pud
Copy link
Contributor Author

e40pud commented Jan 16, 2023

@elasticmachine merge upstream

@e40pud
Copy link
Contributor Author

e40pud commented Jan 16, 2023

@vitaliidm Good catch with the Rule editing page! Fixed that. Also, here is the ticket to cover UI issues mentioned above.

@e40pud e40pud requested a review from vitaliidm January 16, 2023 12:59
@kibana-ci
Copy link
Collaborator

kibana-ci commented Jan 16, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Tests #4 / Add exception using data views from rule details Creates an exception item from alert actions overflow menu
  • [job] [logs] FTR Configs #7 / alerting api integration security and spaces enabled - Group 2 Alerts alerts alerts space_1_all_with_restricted_fixture at space1 should schedule task, run alert and schedule actions when appropriate

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3478 3476 -2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 12.7MB 12.7MB +432.0B

History

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

cc @e40pud

Copy link
Contributor

@vitaliidm vitaliidm 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 addressing comments

Good catch with the Rule editing page!

Some simple e2e tests could've been very handy here. Do you know if we have any tickets to increase tests coverage for rule preview?

@e40pud
Copy link
Contributor Author

e40pud commented Jan 18, 2023

@vitaliidm not that I'm aware of. Here is the ticket that I created to track tests coverage for Rule Preview feature.

@e40pud e40pud merged commit c9130c1 into elastic:main Jan 18, 2023
@kibanamachine kibanamachine added v8.7.0 backport:skip This commit does not require backporting labels Jan 18, 2023
wayneseymour pushed a commit to wayneseymour/kibana that referenced this pull request Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting ci:cloud-deploy Create or update a Cloud deployment release_note:enhancement Team:Detection Alerts Security Detection Alerts Area Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution][Alerts] Make rule preview flyout resizable
7 participants