-
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
Exceptions List component #140985
Exceptions List component #140985
Conversation
@elasticmachine merge upstream |
6a5dd70
to
ae2dba2
Compare
231c0de
to
fa679db
Compare
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 is really awesome and making these components generic will clean up a lot of the exceptions code. Now that we're past 8.5 FF, I think it'd be great to move these into a package. Any potential issues with the package, we have plenty of time to address - but also feel free not to do that in this PR if you want to keep things moving.
Thanks so much for all of this. I'm gonna pull down to play around with it, but none of my comments were anything that would block this from going in.
...tion_engine/rule_exceptions/components/list_common/empty_viewer_state/empty_viewer_state.tsx
Outdated
Show resolved
Hide resolved
...tion_engine/rule_exceptions/components/list_common/empty_viewer_state/empty_viewer_state.tsx
Outdated
Show resolved
Hide resolved
...tion_engine/rule_exceptions/components/list_common/exception_item_card/comments/comments.tsx
Outdated
Show resolved
Hide resolved
...on_engine/rule_exceptions/components/list_common/exception_item_card/exception_item_card.tsx
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_exceptions/components/list_common/exception_items/exception_items.tsx
Outdated
Show resolved
Hide resolved
...olution/public/detection_engine/rule_exceptions/pages/exception_list_details/translations.ts
Outdated
Show resolved
Hide resolved
...olution/public/detection_engine/rule_exceptions/pages/exception_list_details/translations.ts
Outdated
Show resolved
Hide resolved
...ngine/rule_exceptions/pages/exception_list_details/list_with_search/use_list_with_search.tsx
Outdated
Show resolved
Hide resolved
...ty_solution/public/detection_engine/rule_exceptions/pages/exception_list_details/list_api.ts
Outdated
Show resolved
Hide resolved
..._solution/public/detections/pages/detection_engine/rules/all/exceptions/exceptions_table.tsx
Outdated
Show resolved
Hide resolved
fa0f310
to
2102337
Compare
@elasticmachine merge upstream |
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.
@WafaaNasr changes look good but I left some comments related to better typing and simplification which worth to check out.
...engine/rule_exceptions/components/list_common/empty_viewer_state/empty_viewer_state.test.tsx
Outdated
Show resolved
Hide resolved
...tion_engine/rule_exceptions/components/list_common/empty_viewer_state/empty_viewer_state.tsx
Outdated
Show resolved
Hide resolved
...tion_engine/rule_exceptions/components/list_common/exception_item_card/comments/comments.tsx
Outdated
Show resolved
Hide resolved
...ne/rule_exceptions/components/list_common/exception_item_card/conditions/conditions.test.tsx
Outdated
Show resolved
Hide resolved
..._engine/rule_exceptions/components/list_common/exception_item_card/conditions/conditions.tsx
Outdated
Show resolved
Hide resolved
...ptions/components/list_common/exception_item_card/conditions/os_conditions/os_conditions.tsx
Outdated
Show resolved
Hide resolved
...ion_engine/rule_exceptions/components/list_common/exception_item_card/header/header.test.tsx
Outdated
Show resolved
Hide resolved
...ic/detection_engine/rule_exceptions/components/list_common/exception_item_card/meta/meta.tsx
Outdated
Show resolved
Hide resolved
.../detection_engine/rule_exceptions/components/list_common/exception_items/exception_items.tsx
Outdated
Show resolved
Hide resolved
...tion_engine/rule_exceptions/components/list_common/exceptions_utility/exceptions_utility.tsx
Outdated
Show resolved
Hide resolved
@maximpn Thanks for the review, Definitely will work on them once I finish moving all the components into a new Package. |
Pinging @elastic/security-solution (Team: SecuritySolution) |
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Public APIs missing exports
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @WafaaNasr |
packages/kbn-securitysolution-exception-list-components/README.md
Outdated
Show resolved
Hide resolved
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.
Thanks for addressing my previous comments. This looks really great, and so much cleaner! LGTM 🚀
There's a few TODOs that I can address with some upcoming changes once this is in!
Summary
@kbn/securitysolution-exception-list-components
to hold all the Exception-List componentsList
components to be more generic and accept props so they can be placed on any of the Exception pages@emotion/css, @emotion/react
to comply with thekbn-x-packages
Checklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios