-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] [Session View] Session view detail panel and timeline side panel css cleanup #129690
[Security Solution] [Session View] Session view detail panel and timeline side panel css cleanup #129690
Conversation
…meline-side-panel-css
…meline-side-panel-css
}); | ||
|
||
const sessionViewComponent = useMemo(() => { | ||
const sessionViewSearchBarHeight = 118; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a const in session_view/common/constants? I know @opauloh was also hard coding this value in css for his fullscreen fixes. see: https://github.com/elastic/kibana/pull/129257/files#diff-9596f81cf451c350ccfa751c180aa02d876954b8d4ecba208da82a2a17245da5R26
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ideally we'd also have a test to compare the const height with the actual rendered height of the toolbar, but that can always come later :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya I think we should, but probably in a different pr and remove both magic-ish numbers, just to be sure this gets in the next BC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the one minor suggestion, but otherwise LGTM!
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…line side panel css cleanup (#129690) (#129784) * Make session detail content scroll if needed * Remove unneeded importants and hard coded light colors, make theme aware * Use full screen styles, make the dualing flyouts work together * Remove unused import (cherry picked from commit 9f305da) Co-authored-by: Kevin Qualters <[email protected]>
Summary
Resolves #128960 and #129532. Also removes a hard coded light color for a color that works with both light and dark themes, and removes some unneeded importants. Leaving in draft to discuss alternatives to having the scroll behavior for the detail pop over come from security solution, as it would be more consistent with the existing flyout if it was handled in the session view. I think it would make more sense for a wrapper around the expandable accordions to be the part of the document that scrolls, inline with the table and json view in the timeline flyout.
Screen.Recording.2022-04-07.at.12.18.08.PM.mov