-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Timeline] extract and cleanup timeline bottom bar component #173886
[Security Solution][Timeline] extract and cleanup timeline bottom bar component #173886
Conversation
aa086fb
to
0dca91d
Compare
acca57e
to
aa2075d
Compare
Files by Code Ownerelastic/kibana-localization
elastic/protections-experience
elastic/security-defend-workflows
elastic/security-engineering-productivity
elastic/security-solution
elastic/security-threat-hunting-investigations
|
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 - From Defend Workloads team 👍
x-pack/plugins/security_solution/public/timelines/components/flyout/header/index.tsx
Outdated
Show resolved
Hide resolved
@PhilippeOberti What's the advantage of using |
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.
Defend workflows lgtm 👍
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.
one nit, feel free to ignore it but ternaries are something I dont like :D
x-pack/plugins/security_solution/public/timelines/hooks/use_timeline_title.tsx
Outdated
Show resolved
Hide resolved
@janmonschke The reason I chose a hook over a component here was because I specifically did not want to have any display/rendering in it. That hook is used in 2 places: the timeline bottom bar and the header of the timeline modal. Both display the same value, but the way they are displayed is different:
That might not be a good enough reason to have it a hook vs a component though, not sure honestly... I just feel like I would hate having a component with an if/else condition to handle those 2 situations, and I also dislike the option of passing the some css or classname as props to drive the display. |
After talking to the team during standup, I'm going to create a selector for this instead. No need of a component or a hook! :) |
aa2075d
to
e8449dd
Compare
implemented here |
9f4c86e
to
5e7c8ab
Compare
...n_cypress/cypress/e2e/detection_response/detection_engine/sourcerer/sourcerer_timeline.cy.ts
Show resolved
Hide resolved
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.
Cypress tests for sourcerer LGTM!
x-pack/plugins/security_solution/public/timelines/store/selectors.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/store/selectors.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/timelines/store/selectors.ts
Outdated
Show resolved
Hide resolved
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.
🎉
5e7c8ab
to
3a83646
Compare
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Notes
This is a follow up PR to #173880 which should be merged first. The changes can be reviewed individually though, by ignoring the first 2 commits and focusing only on the third one.
This PR is part of a long-running effort to clean up the timeline code. I originally had opened #172408 but the file count kept growing and so did the complexity for the reviewers. Of that original PR I will create probably 5-6 smaller ones.
Summary
This very PR focuses on separating the
FlyoutBottomBar
(the timeline bottom bar) from theStatefulTimelineComponent
(loaded inside the modal). Those 2 components were using the sameFlyoutHeaderPanelComponent
but the content of that component wasn't actually common and lots of if/else condition existed to support both usages.First extract the FlyoutBottomBar component:
flyout
folder as it is actually loaded within the timeline wrapper component, not the flyoutbottom_bar
andTimelineBottomBar
respectively to better represent what the component actually doesFlyoutHeaderPanelComponent
directly, which doesn't really make much senseThe PR also removes the ActiveTimelines component which was used in the
FlyoutHeaderPanelComponent
but isn't needed anymore, as the modal header can render the title directly.Also a new
useTimelineTitle
is introduced to always return the correct timeline title (handles saved, draft and template states)Absolutely no visual or behavior changes introduced here!
Checklist
The following PR will focus on cleanup up the
flyout
folder.