-
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] Updates the text in the Actions log and Status command to match UX #135935
Conversation
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
This is looking great! 🔥 Left just minor suggestions
*/ | ||
|
||
import { i18n } from '@kbn/i18n'; | ||
import { HostStatus } from '../../../../common/endpoint/types'; |
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.
import { HostStatus } from '../../../../common/endpoint/types'; | |
import type { HostStatus } from '../../../../common/endpoint/types'; |
import { i18n } from '@kbn/i18n'; | ||
import { HostStatus } from '../../../../common/endpoint/types'; | ||
|
||
export const agentStatusText = (hostStatus: HostStatus) => { |
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.
Since there is no JSX code here this file can be a .ts
extension.
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.
Name suggestion: getAgentStatusText
@@ -129,7 +130,7 @@ export const CommandUsage = memo<CommandUsageProps>(({ commandDef }) => { | |||
}, [commandDef.args, hasArgs]); | |||
|
|||
const parametersDescriptionList = (title: string, parameters: CommandDetails) => { | |||
const description = parameters.map((item) => ( | |||
const description = parameters.map((item, index) => ( |
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.
index
is not needed here
/> | ||
</EuiText> | ||
<EuiText> | ||
const statusDescriptionList = () => { |
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.
I would suggest you move this inside a useCallback
and change the name by something like getStatusDescriptionList
}`; | ||
}; | ||
|
||
const statusData = [ |
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.
This has also a dynamic result, move it in a useMemo
will avoid unnecessary code execution during renders.
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.
@dasansol92 if I wrap the containing function into the useCallBack()
and the deps
array contains the dynamic data, won't this be taken care of? If I put this as useMemo()
we'd have a hook in a hook.
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.
Ohh! You are totally right! Github diff hided me the useCallback
. No need for useMemo
here then. Apologies for the noise.
@@ -164,38 +168,39 @@ export const ResponseActionsList = memo< | |||
: undefined; | |||
|
|||
const command = getCommand(_command); | |||
const descriptionListLeft = [ | |||
const dataList = [ |
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.
This and outputList
have a dynamic result, you can use useMemo
for those.
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.
same for these, they're already wrapped in a useCallBack()
with a deps
array, would it still be necessary to wrap these in useMemo()
?
x-pack/plugins/security_solution/public/management/components/console/console.tsx
Show resolved
Hide resolved
} | ||
dt { | ||
font-weight: ${(props) => props.theme.eui.euiFontWeightSemiBold}; | ||
margin-top: ${(props) => props.theme.eui.euiSizeS} !important; |
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.
This line is redundant. line 63 already covers both dt
and dd
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.
@ashokaditya see this commit: 2bd50b7#diff-56e3d3d36e2892512e4de0a61b7e79f3a5855544e3ec6adb480c85fec616fde2R60
I refactored this a bit to remove the need for !important
@elasticmachine merge upstream |
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, thanks for the changes!
…nlog/kibana into task/status-action-log-text
x-pack/plugins/security_solution/public/common/components/endpoint/agent_status.tsx
Show resolved
Hide resolved
import { i18n } from '@kbn/i18n'; | ||
import type { HostStatus } from '../../../../common/endpoint/types'; | ||
|
||
export const getAgentStatusText = (hostStatus: HostStatus) => { |
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.
Ok. I see what you did here... When I saw getAgentStatusText()
I though you were using the Fleet helper function.
Since this is under common/components/
, can you make it a component? as in:
<EndpointAgentStatusText hostStatus="" />
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.
My original intention was just to have this generate the text for the status which can be used either in plain text reporting or in a containing component like it is here: https://github.com/elastic/kibana/pull/135935/files#diff-dcba59229f5aeb7b5e1cc30e8b37d4b848460e6e240da640eac479b392e28939R21
Thought - are you OK if I export this text from the above file so that we can either import the text or the entire AgentStatus
component based on the need?
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.
yeah, that's fine - nothing that should hold this up. It just feels more like a "service" or "utility" than a component, so I was just highlighting that it probably should not be under component/
@@ -115,6 +115,114 @@ export const EndpointStatusActionResult = memo< | |||
} | |||
}, [fetchedPendingActionsSummary, setStore]); | |||
|
|||
const getStatusDescriptionList = useCallback(() => { |
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 FYI: This would be more efficient if it was a useMemo()
that would only be updated if the dependencies changed. The way it is now, its called every time there is a re-render of the component
const agentStatus = () => { | ||
let isolateStatus = ''; | ||
|
||
if (pendingIsolationActions.pendingIsolate > 0) { |
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.
Let's not hold up on this PR, but this logic here for actions no longer makes sense now that we introduced more actions. Not sure that isolateStatus
should be set to isolating
or releasing
when there are other actions also pending.
(FYI: I also found that our component that normally show the isolation status/pending actions is not showing all pending actions in the tooltip - opening issue now)
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.
Checked out PR and looked over status
command. Looks good 👍
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
…mmand to match UX (elastic#135935)
Summary
This PR updates the text in the Action log tray and status command to match the mocks and use a uniform text. Also readjusted some spacing for the help command after feedback from UX.
Action log:
Status command:
Readjusted help command spacing:
Checklist
Delete any items that are not applicable to this PR.