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] [Findings] Adding grouping component #169884
[Cloud Security] [Findings] Adding grouping component #169884
Changes from 15 commits
8bbb01a
c40e2fc
0ac7e9e
bb7f308
1bacb6a
c9f251e
9432b87
4fd1510
fd4e071
40a7797
bd80f34
befa489
2f761d5
ec0dbdf
d55081c
4d8e9bc
59cf7e3
5abda98
dcde804
97c19a3
be967d2
199f938
f51a462
1af2377
1eb9ef2
a145184
24188a3
975fd13
1c0308c
7bacb48
ca4713c
95f8cf4
70e9539
2c35de7
3d866de
dca18ba
af4d6b5
6ff86ab
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.
should we keep the behaviour of disabling currently selected option when the max level is 1? Right now to remove grouping, you can either switch to none or "unselect" the option. If we change the behavior to toggle when max level is 1, maybe it makes sense to disable this "unselect" behaviour
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.
great point, I'll adjust it in the code and bring it to discussion with the code owners
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.
thats fine, can you please just add some unit tests? We never get to line 53 in the current tests
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 added the unit tests related to the changes introduced in this PR, thanks a lot for reviewing it @stephmilovic!
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 think
kbn-securitysolution-grouping
lib would benefit from having some unit tests. As it didn't have any, I wouldn't block this PR on that, but I'd vote for adding some of the tests which are testing your changes to the libThere 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.
@maxcold Why do you think there are not unit tests? We do have tests that should have been added to :
packages/kbn-securitysolution-grouping/src/components/group_selector/index.test.tsx
packages/kbn-securitysolution-grouping/src/hooks/use_get_group_selector.test.tsx
Please add unit tests
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.
@stephmilovic totally my bad, I have no idea where I looked tbh, but I remember checking the codebase in my code editor and not finding the unit test files. Some kind of a blackout from my side I guess :) for sure we need to cover new cases with unit tests
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 looks to be the only untested condition here
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.
test added 🙌