-
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
[Security Solution] Add VersionPicker
component
#188302
Conversation
VersionPicker
componentComparisonSide
component
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
@nikitaindik Can you add a link to this PR to your TODO list in #171520? Eventually there's gonna be lots of PRs we'd probably need revisiting in the future. |
1c21c5a
to
5f33ce3
Compare
ComparisonSide
componentVersionPicker
component
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.
Very cool @nikitaindik, thank you for exploring and integrating with Storybook -- reviewing this PR was so easy and nice experience!
...e/rule_management/components/rule_details/three_way_diff/versions_picker/versions_picker.tsx
Outdated
Show resolved
Hide resolved
...n_engine/rule_management/components/rule_details/three_way_diff/versions_picker/constants.ts
Outdated
Show resolved
Hide resolved
export function VersionsPicker({ | ||
hasBaseVersion, | ||
selectedVersions, | ||
onChange, | ||
}: VersionsPickerProps) { |
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.
Nit: can we agree on a canonical way of defining components? The previous one was defined as an FC<...>
:
export const CustomizedPrebuiltRuleBadge: React.FC<CustomizedPrebuiltRuleBadgeProps> = ({
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 don't have a strong preference. One very minor downside of using React.FC
is that it adds children
prop to type even for childless components like this one. In React v18 children
are removed. We are on v17 now.
function
-style components are a slightly more readable and can be ordered in any way, so they are a bit more flexible.
In the future we could probably add an ESLint rule to enforce for canonical way if we decide what the way is. But I am fine with either approach.
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.
@nikitaindik I'm fine with either too. Could be a bikeshedding topic for Tech Time when we won't have an agenda :)
...anagement/components/rule_details/three_way_diff/versions_picker/versions_picker.stories.tsx
Outdated
Show resolved
Hide resolved
💔 Build Failed
Failed CI StepsMetrics [docs]
cc @nikitaindik |
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]
To update your PR or re-run it, just comment with: cc @nikitaindik |
Partially addresses: #171520
Summary
This PR adds the
VersionPicker
component for the ThreeWayDiff UI. This component is a part of theComparisonSide
component (see it on the Miro diagram).ComparisonSide
will display the read-only diff between two selected field versions.This component is not yet connected to the Upgrade flyout. You can view and test it in Storybook by running
yarn storybook security_solution
in the root Kibana dir. Go tohttp://localhost:9001
once the Storybook is up and running.