-
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
Value lists modal window #179339
Value lists modal window #179339
Changes from 55 commits
ca305c4
e1c1d5b
b149787
fcf4599
0bd4fde
03cc414
b97ba75
239e18f
624d0a6
585be6d
db3b98f
aeb4a42
f9653df
b537ca1
08a4328
8f37ed8
8f25972
07cf360
527c02e
dca48b9
cc2bdc0
4a13eab
4b200f8
ead589a
9190c63
d204d79
0a76ae7
e7593d1
7d65667
23b71db
8a6893a
ddb9d46
0a917f0
8ecaf9e
c5a9217
aa149b1
af57a88
a1abdd5
e1fa6f6
3f0a040
a1f925b
4e20033
abe68f8
9492564
3158861
72690ea
5535ed4
661c0f4
2481ff3
edc9892
b5bcf2e
5168e0c
a1aa82f
4455600
83df32a
5da233d
47eec14
c9deda4
9b030df
a74fe9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ | |
* Side Public License, v 1. | ||
*/ | ||
|
||
import React from 'react'; | ||
import React, { ElementType } from 'react'; | ||
import { css } from '@emotion/css'; | ||
import { EuiExpression, EuiBadge } from '@elastic/eui'; | ||
import type { ListOperatorTypeEnum } from '@kbn/securitysolution-io-ts-list-types'; | ||
|
@@ -21,13 +21,21 @@ const entryValueWrapStyle = css` | |
const EntryValueWrap = ({ children }: { children: React.ReactNode }) => ( | ||
<span className={entryValueWrapStyle}>{children}</span> | ||
); | ||
const getEntryValue = (type: string, value?: string | string[]) => { | ||
|
||
const getEntryValue = (type: string, value: string | string[], showValueListModal: ElementType) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing here: I don't think this needs to be an argument to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I totally agree and I don't like this props drilling myself. The problem that those Exceptiond and Autocomplete are - packages - and only way I found it's to pass this modal window as props. We already do that, and have those comments
Alternative solutions - can be to move this modal window to another package, and make as dependency for Exception and Autocomplete - But I don't see why a very specific business logic component should go to another package. Please let me know if you have other ideas There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, after going through the code again, and getting better acquainted with the different packages we have, it does feel like these value list components should be their own package (so as to avoid the circular dependency between It would be great to do that here and avoid the churn in this PR, but if you'd prefer to do that in a subsequent PR I could probably be convinced of that. Let me know what you think. cc @yctercero There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But: if you do end up sticking with the current props-based solution, we should at least make it an FC with the correctly-typed props. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this is what I'm thinking:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After discussion today, we decided that we will not move Value list modal to another packages, as it already depends on a lot of things from security-solution plugin. Otherwhise there was idea to move Exceptions component package back into security solution, which I will create issue for. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agree, but don't know what other options we can have here. Can be expanded flyout, but exceptions flyout already to wide, it will be not enough place for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
would run some flaky test runner https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/5657 |
||
const ShowValueListModal = showValueListModal; | ||
if (type === 'match_any' && Array.isArray(value)) { | ||
return value.map((currentValue, index) => ( | ||
<EuiBadge key={index} data-test-subj={`matchAnyBadge${index}`} color="hollow"> | ||
<EntryValueWrap>{currentValue}</EntryValueWrap> | ||
</EuiBadge> | ||
)); | ||
} else if (type === 'list' && value) { | ||
return ( | ||
<ShowValueListModal shouldShowContentIfModalNotAvailable listId={value.toString()}> | ||
{value} | ||
</ShowValueListModal> | ||
); | ||
} | ||
return <EntryValueWrap>{value}</EntryValueWrap> ?? ''; | ||
}; | ||
|
@@ -48,12 +56,13 @@ export const getValue = (entry: Entry) => { | |
export const getValueExpression = ( | ||
type: ListOperatorTypeEnum, | ||
operator: string, | ||
value: string | string[] | ||
value: string | string[], | ||
showValueListModal: ElementType | ||
) => ( | ||
<> | ||
<EuiExpression | ||
description={getEntryOperator(type, operator)} | ||
value={getEntryValue(type, value)} | ||
value={getEntryValue(type, value, showValueListModal)} | ||
data-test-subj="entryValueExpression" | ||
/> | ||
<ValueWithSpaceWarning value={value} /> | ||
|
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.
Is there a reason that this is a prop of
AutocompleteFieldListsComponent
? The interface doesn't look very flexible (or typed correctly, since we pass very specific props toshowValueListModal
), and I don't see any data being reported back up... I would just delete lines 39, 59, and 123 👍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.
That would also get rid of many of the changes on this branch 😉