-
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
[Cloud Security] Rules Combo Box filters Custom component #175175
[Cloud Security] Rules Combo Box filters Custom component #175175
Conversation
/ci |
/ci |
Pinging @elastic/kibana-cloud-security-posture (Team:Cloud Security) |
/ci |
BulkActionBenchmarkRulesResponse, | ||
} from './v4'; | ||
|
||
const DEFAULT_BENCHMARK_RULES_PER_PAGE = 25; |
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'm not sure why this or any other instances of this value are kept here and are basically versioned. i dont see a reason
export type FindCspBenchmarkRuleRequest = TypeOf<typeof findCspBenchmarkRuleRequestSchema>; | ||
|
||
export interface BenchmarkRuleSelectParams { | ||
section?: string[]; | ||
ruleNumber?: string[]; | ||
} |
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.
types at the top
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.
not sure if I understand this comment ?
|
||
const DEFAULT_BENCHMARK_RULES_PER_PAGE = 25; | ||
|
||
export const findCspBenchmarkRuleRequestSchema = schema.object({ |
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.
can you highlight what is new here that requires a new version?
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 ruleNumber and section, previously its only string because we can only filter using 1 rule number and 1 cis section. now its an array because can filter using multiple rule number and cis section
@@ -218,3 +218,29 @@ export const getBenchmarkFilterQuery = ( | |||
: ''; | |||
return baseQuery + sectionQuery + ruleNumberQuery; | |||
}; | |||
|
|||
export const getBenchmarkFilterQueryV2 = ( |
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 like we get the same params and return a string, why do we need a new version?
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.
previously we only accept string for the rule number and section filter, but now since we add ability to filter based on multiple rule number and/or section we use array instead of string
selectedOptionKeys?: string[]; | ||
transparentBackground?: boolean; | ||
} | ||
export const MultiSelectFilter = <T extends string, K extends string = string>({ |
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.
please add a comment with a link to the original code so we can reference to it in the future if we need to
@@ -190,7 +190,7 @@ const getColumns = ({ | |||
}} | |||
/> | |||
), | |||
width: '30px', | |||
width: '3%', |
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.
percentage are subject to change, on my ultrawide monitor it would be too much. any reason not to use pixels 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.
ah kk, no reason regarding the not using pixels, I just thought to keep it consistent with all the other rows (all other rows are using percentage)
@@ -255,13 +255,13 @@ const getColumns = ({ | |||
name: i18n.translate('xpack.csp.rules.rulesTable.mutedColumnLabel', { | |||
defaultMessage: 'Enabled', | |||
}), | |||
width: '10%', | |||
width: '5%', |
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 here, both of there columns present a UI component with a set width. a switch and a checkbox, they can both use pixels
truncateText: true, | ||
render: (name, rule: CspBenchmarkRulesWithStates) => { | ||
const rulesObjectRequest = { | ||
benchmark_id: rule?.metadata.benchmark.id, | ||
benchmark_version: rule?.metadata.benchmark.version, | ||
/* Since Packages are automatically upgraded, we can be sure that rule_number will Always exist */ | ||
/* Rule number always exists* from 8.7 */ |
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.
/* Rule number always exists* from 8.7 */ | |
/* Rule number always exists from 8.7 */ |
onChange={(option) => { | ||
setSelectedSection(option); | ||
onSectionChange(option.length ? option[0].label : undefined); | ||
id={'section'} |
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.
maybe something like cis-section-multi-select-filter
onChange={(option) => { | ||
setSelectedRuleNumber(option); | ||
onRuleNumberChange(option.length ? option[0].label : undefined); | ||
id={'rule-number'} |
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.
maybe something like rule-number-multi-select-filter
@@ -190,7 +193,7 @@ const getColumns = ({ | |||
}} | |||
/> | |||
), | |||
width: '3%', | |||
width: '40px', |
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.
40px seems too much for a 16x16 checkbox, can you add a screen shot?
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 have updated the screenshot 👍
css={css` | ||
padding-left: 18px; | ||
`} |
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.
why? can you add a screenshot? i think making the column the same width as the widget will be better
@@ -218,3 +218,29 @@ export const getBenchmarkFilterQuery = ( | |||
: ''; | |||
return baseQuery + sectionQuery + ruleNumberQuery; | |||
}; | |||
|
|||
export const getBenchmarkFilterQueryV2 = ( | |||
id: BenchmarkId, |
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.
id: BenchmarkId, | |
benchmarkId: BenchmarkId, |
|
||
export const getBenchmarkFilterQueryV2 = ( | ||
id: BenchmarkId, | ||
version?: string, |
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.
version?: string, | |
benchmarkVersion?: string, |
section: schema.maybe(schema.string()), | ||
ruleNumber: schema.maybe(schema.string()), | ||
section: schema.maybe(schema.arrayOf(schema.string())), | ||
ruleNumber: schema.maybe(schema.arrayOf(schema.string())), |
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 reason to modify the pervious version?
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.
oh good catch this wasnt supposed to be modified
FindCspBenchmarkRuleResponse, | ||
} from '../../../../common/types/latest'; | ||
import type { FindCspBenchmarkRuleRequest } from '../../../../common/types/rules/v4'; |
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.
CspBenchmarkRule
and FindCspBenchmarkRuleResponse
also should be imported from v4.
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
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* main: (520 commits) Update Kibana code editor dependencies (#171720) [SLOs] Hide view in app in slo alerts table in slo details page (#175441) [api-docs] 2024-01-25 Daily api_docs build (#175502) [DOCS] Add buildkite links to doc preview comments (#175463) skip flaky suite (#175443) [Security Solution][Timeline] refactor timeline modal save timeline button (#175343) [RAM] Stack Management::Rules loses user selections when navigating back (#174954) [Security Solution][Timeline] refactor timeline modal attach to case button (#175163) Upgrade EUI to v92.1.1 (#174955) [Fleet]: Beta label is shown inconsistently while selecting proxy under Fleet settings. (#170634) [Cloud Security] Rules Combo Box filters Custom component (#175175) skip flaky suite (#175407) [Security Solution][Timeline] refactor timeline modal open timeline button (#175335) [Embedded Console] Introduce kbnSolutionNavOffset CSS variable (#175348) [Console] disable access to embedded console without dev tools capability (#175321) fix(x-pack/reporting): use FIPS-compliant ID generator `uuidv4` in Reporting plugin (#174809) [Security Solution] Data quality dashboard persistence (#173185) [RAM][Observability] Add alert fields table to Observability flyout (#174685) test: add missing await for connector table disappearance (#175430) [RAM][Maintenance Window] Fix maintenance window FE types and transforms (#173888) ...
…5175) ## Summary This PR is for updating the current find rule API to allow multi select and search on section and rule numbers it also add a custom component for the section and rule number filter also some small UI fixes <img width="1486" alt="Screenshot 2024-01-24 at 4 22 35 AM" src="https://github.com/elastic/kibana/assets/8703149/c78987a0-b4a8-4dcc-b1fb-71ade0853340">
Summary
This PR is for updating the current find rule API to allow multi select and search on section and rule numbers
it also add a custom component for the section and rule number filter
also some small UI fixes