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
[Cloud Security] Show graph visualization in expanded flyout #198240
[Cloud Security] Show graph visualization in expanded flyout #198240
Changes from 44 commits
a982ad7
c94c04a
5c00b6e
1589ef8
6f71b19
3dc4523
b88b074
dbb2688
008521f
70ee31d
dbab169
78246a7
5318512
74ecb0e
b57dfac
2d0d360
f94db1e
0899098
702ef00
1ad2ce4
30f73c8
3821ad9
33134f6
c98ab19
708300d
34656ae
b0f73b9
c7fb622
5c50026
6969f69
21d04e2
fef5ea3
6b81c5d
e8c6367
4b1512e
b13bc2a
2f0ea4a
20be49b
0b52af2
bcbe7f6
dc5bb1c
1f0bcec
6ac1a58
5e90b2c
6085cbd
6a55f50
40e66c4
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.
as far as I can see everywhere the
useFetchGraphData
hook is called, therefetchOnWindowFocus
is set to false? Should it be the default value then?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 kept it false to be consistent with react-query defaults
wdyt?
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.
What is the logic behind different values for
keepPreviousData
? From the UX perspective I didn't notice any difference when changed tofalse
here with the mocked data? If there is some edge case we want to cover by this option, it might worth a commentThere 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.
the logic is that while you query you will see the previous results.
if it is set to false, on every query your graph will be cleaned, and when the response is back it will be drawn. Which creates a flicker.
Besides that, on error you will lose your current graph data. instead of seeing your previous graph data.
I think it provides a better UX
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.
what this logic of combined vs phrase filter is about? not really sure, maybe also worth a comment explaining the edge cases and/or add unit tests wich would explain different scenarious
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.
so we keep always a single filter with
OR
statementsSo whenever addFilter is called, it checks first if it exists or not, and then adds OR statement when needed
I'll add comments now and UT later, once I'll have graph_investigation in storybook (it requires some extra mocking and this PR is already quite big).
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.
any reason not to add
expandButtonClickHandler
to the deps map?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, it stops behaving as expected :P
(seriously, when I remove the filters it renders an empty graph although the graph api response is not empty)
I honestly don't remember why atm. But we can try to invest more time to understand why it causes issues