-
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][Cases] Fix cases breadcrumbs #97040
[Security Solution][Cases] Fix cases breadcrumbs #97040
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
@@ -80,18 +80,19 @@ Arguments: | |||
|createCaseNavigation|`CasesNavigation` route configuration for create cases page | |||
|getCaseDetailHrefWithCommentId|`(commentId: string) => string;` callback to generate the case details url with a comment id reference from the case id and comment id | |||
|onComponentInitialized?|`() => void;` callback when component has initialized | |||
|ruleDetailsNavigation|: `CasesNavigation<string | null | undefined, 'configurable'>` |
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.
Only changes here were escaping some errant pipes |
and removing some errant colons :
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.
nice, i swear i tried escaping but guess I did a bad job ¯_(ツ)_/¯
@@ -64,6 +61,7 @@ export interface CaseViewComponentProps { | |||
|
|||
export interface CaseViewProps extends CaseViewComponentProps { | |||
timelineIntegration?: CasesTimelineIntegration; | |||
nonVisibleRenderWithCaseData: (props: { caseData: Case }) => JSX.Element | null; |
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.
still have a JSX.Element
in the return given SpyRoute
Hey! Thanks for the fix. I don't understand why cases should bother with the breadcrumb? Shouldn't be the responsibility of the security solution plugin? I think the cases component should be as pure as possible. I think we can do it as:
and remove the |
I 1000% agree @cnasikas. I really don't like |
I see what you mean. I knew there was a good reason but I missed that. It feels awkward that is a component and not a callback function. That's why it filled strange to me and wanted to pointed out. What about having this prop Example:
|
I like 👍🏾 |
2d567f1
to
ee1b42c
Compare
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.
Nicely done! Looks like @cnasikas beat me to the review but I'll go ahead and take those internet points anyways. Very clean, I like. Thanks to you both 🏁
💚 Build SucceededMetrics [docs]
History
To update your PR or re-run it, just comment with: |
Summary
This PR fixes the broken breadcrumbs in the new cases plugin, and makes some minor fixes to the README. Instead of adding a spyRoute component, just generalized it to nonVisibleRenderWithCaseData (naming is hard... 😂). Totally up for suggestions on how this can be improved