-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Resolver] - Maintain active node #86682
[Security Solution][Resolver] - Maintain active node #86682
Conversation
@elasticmachine merge upstream |
1 similar comment
@elasticmachine merge upstream |
5a2d303
to
dbc5570
Compare
@elasticmachine merge upstream |
payload: nodeID, | ||
payload: { | ||
nodeID, | ||
time: timestamp(), |
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 need the time
field? I probably missed something but doesn't look like we use it in the reducer right?
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.
it's used by the animatePanning helper in the reducer https://github.com/elastic/kibana/pull/86682/files#diff-f9c46528634fc1f01e36cc14d7ebcfad609a0ce59c2fcc081520f8e65c0bbc18R69
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.
Yea, this line specifically:
camera: animatePanning(nextState.camera, action.payload.time, position, animationDuration), |
import { urlSearch } from '../test_utilities/url_search'; | ||
|
||
const resolverComponentInstanceID = 'useSyncSelectedNodeTestInstanceId'; | ||
const animationDuration = 1000; |
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 can't remember if it's possible but should we import the duration amount from the resolver code?
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.
We never made this a constant anywhere, but just did so and updated it in any relevant tests
89775c0
to
5e82862
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.
Looks good, just wasn't sure if the cypress changes should be removed.
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Distributable file count
History
To update your PR or re-run it, just comment with: |
Spoke with @patrykkopycinski and it should remain in there. Reverted my changes to the |
Co-authored-by: Kibana Machine <[email protected]>
…87371) Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Pinging @elastic/endpoint-app-team (Feature:Resolver) |
Summary
This PR removes the deprecated
userBroughtNodeIntoView
, and adds auseSyncSelectedNode
to sync the active node to any url changes that take place via interaction with the graph as well as via the browser back and forward buttons. This also resolves #81189GIF of the forward and back actions below
syncSelected.gif.zip
Checklist
Delete any items that are not applicable to this PR.