Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Security Solution] Updates the text in the Actions log and Status command to match UX #135935
Changes from all commits
0a2773a
ee6c089
681e121
8244a73
9b26a65
013cdd0
84a9842
788df06
2bd50b7
6930af9
c39f25b
2fef0af
107effc
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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: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/
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 componentThere 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 toisolating
orreleasing
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.
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 thedeps
array contains the dynamic data, won't this be taken care of? If I put this asuseMemo()
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 foruseMemo
here then. Apologies for the noise.