-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Various Cases cleanups #102103
Conversation
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
files: ['x-pack/plugins/cases/**/*.{js,mjs,ts,tsx}'], | ||
rules: { | ||
'arrow-body-style': ['warn', 'as-needed'], | ||
'no-duplicate-imports': 'error', |
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.
🎉
block: jest.fn(), | ||
createHref: jest.fn(), | ||
listen: jest.fn(), | ||
push: () => {}, |
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.
Do we still have some tests using this mock now that we removed them from the file below?
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.
good point no i dont think so, there is one but its import routeData from 'react-router';
. ill take care of it
x-pack/plugins/cases/public/components/user_action_tree/index.test.tsx
Outdated
Show resolved
Hide resolved
useEffect(() => { | ||
return () => { | ||
useEffect( | ||
() => () => { |
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.
nit: this is probably just me but for this specific instance I find this harder to read 🤷♂️
@@ -18,7 +18,9 @@ import * as i18n from './translations'; | |||
|
|||
export interface CasesNavigation<T = React.MouseEvent | MouseEvent | null, K = null> { | |||
href: K extends 'configurable' ? (arg: T) => string : string; | |||
onClick: (arg: T) => void; | |||
onClick: K extends '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.
Should we also add void | Promise<void>
since in some cases this can be an async function?
x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx
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.
limits.yml change LGTM
@elasticmachine merge upstream |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Various Cases cleanups. Some for performance reasons, other for preference to shorten code:
useHistory
fromreact-router-dom
to usenavigateToApp
, and removedRouter/mockHistory
from tests <- this is probably the most important piece to reviewuseMemo
/useCallback
where it should've been in the first place"arrow-body-style": ["error", "as-needed"]
and then removed because I realized prettier will cancel the rule out. I left in the commit with 23 file changes just because I struggled to remove the commit. sorry about that reviewer.¯\_(ツ)_/¯
The big change that made our
limits.yml
size go down was removing unused translations. I am going to be making more performance related changes in a future PR, especially around translations, but wanted to post what I had so far in a PR since it touches a lot already.