Skip to content

Refactor Rules Page#146193

Merged
CoenWarmer merged 5 commits intoelastic:mainfrom
CoenWarmer:chore/refactor-rules-page
Nov 24, 2022
Merged

Refactor Rules Page#146193
CoenWarmer merged 5 commits intoelastic:mainfrom
CoenWarmer:chore/refactor-rules-page

Conversation

@CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Nov 23, 2022

Resolves #156985

Summary

This PR aims to refactor the Rules page for more maintainability and better legibility.

Functionally the page should behave exactly the same.

Guidelines while refactoring:

  • Consistent use of page title components (pageTitle: string | component, rightSideItems: [<HeaderActions />])
  • Break up components to make them easier to read, mock, test
  • Separation of concerns
  • Place JSX defined before the return of the component in its own component
  • Reusable business logic into hooks
  • Reuse newly created hooks
  • Functions that are exported and only used once in the codebase don't have to be their own functions (yet), they can be defined inside the calling component

Part of refactoring effort of other AO pages:

@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Nov 23, 2022
@CoenWarmer CoenWarmer changed the title Refactor Rules page Refactor Rules Page Nov 23, 2022
@CoenWarmer CoenWarmer marked this pull request as ready for review November 24, 2022 11:54
@CoenWarmer CoenWarmer requested a review from a team November 24, 2022 11:54
@kibana-ci
Copy link

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 436 437 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
observability 495.7KB 495.7KB -14.0B
Unknown metric groups

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 59 65 +6
observability 46 45 -1
osquery 109 115 +6
securitySolution 443 449 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
observability 53 52 -1
osquery 110 117 +7
securitySolution 520 526 +6
total +20

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @CoenWarmer for this refactoring. Definitely make the code easier to follow 👍🏻

ruleTypeIndex,
});

return render(<RulesPage />);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️ testing-library.
Is it what we (elastic) are going with moving forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't venture to guess if we are going with RTL in general, but I do know that Enzyme is considered EOL (https://dev.to/wojtekmaj/enzyme-is-dead-now-what-ekl) so I think it makes sense to move our tests to RTL.

{createRuleFlyoutVisibility && CreateRuleFlyout}
<EuiFlexGroup direction="column" gutterSize="s">
<EuiFlexItem>
<RuleList
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✨ Nice, easier to read like this 👍🏻

@CoenWarmer CoenWarmer merged commit f063ba4 into elastic:main Nov 24, 2022
@CoenWarmer CoenWarmer deleted the chore/refactor-rules-page branch November 24, 2022 17:33
@kibanamachine kibanamachine added v8.7.0 backport:skip This PR does not require backporting labels Nov 24, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 28, 2022
* main: (30 commits)
  [Cloud Posture] test latest findings table sort (elastic#144668)
  [api-docs] 2022-11-28 Daily api_docs build (elastic#146359)
  [api-docs] 2022-11-27 Daily api_docs build (elastic#146353)
  [api-docs] 2022-11-26 Daily api_docs build (elastic#146350)
  [DataViews] Fix form validation UX when the same data view name already exists (elastic#146126)
  [Discover] Prevent agg based visualizations of Discover saved objects with adhoc data views (elastic#145583)
  [Health Gateway] Update response aggregation (elastic#145761)
  [api-docs] 2022-11-25 Daily api_docs build (elastic#146341)
  [Metric threshold rule] Adds new context variable for group by keys (elastic#145654)
  [Controls] [Portable Dashboards] Add control group renderer example plugin (elastic#146189)
  Refactor Observability Overview Page (elastic#146182)
  Send complete test data to xMatters, so it can create an alert (elastic#145431)
  [Dashboard] [Controls] Allow options list suggestions to be sorted (elastic#144867)
  Add open API specification for list connector types (elastic#145951)
  skip flaky suite (elastic#146086)
  [ML] Removing duplicate tooltip text (elastic#146308)
  Refactor Rules Page (elastic#146193)
  [DOCS] Alert limit for cases (elastic#145950)
  Extend session index fields mapping with a session creation timestamp. (elastic#145997)
  [Files] Move <Image /> component to `@kbn/shared-ux` package (elastic#145995)
  ...
@CoenWarmer CoenWarmer mentioned this pull request May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.7.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor Rules page

4 participants