Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor Observability Overview Page #146182

Merged
merged 11 commits into from
Nov 24, 2022
Merged

Refactor Observability Overview Page #146182

merged 11 commits into from
Nov 24, 2022

Conversation

CoenWarmer
Copy link
Contributor

@CoenWarmer CoenWarmer commented Nov 23, 2022 •
edited
Loading

Resolves #156986

Summary

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

Functionally the page should behave exactly the same.

It is recommended to review this PR commit by commit.

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 requested review from a team as code owners November 23, 2022 16:24
@CoenWarmer CoenWarmer added the release_note:skip Skip the PR/issue when compiling release notes label Nov 23, 2022
This was referenced Nov 23, 2022
Merged
Closed
@formgeist
Copy link
Contributor

@CoenWarmer This seems out of scope for a @elastic/observability-design review in terms of code refactor. Can you seek a review from an engineer within @elastic/observability-ui to review this PR?

@formgeist
Copy link
Contributor

@CoenWarmer Design will take a look for possible visual regressions, but we don't have the knowledge to review the code itself.

@CoenWarmer
Copy link
Contributor Author

@CoenWarmer This seems out of scope for a @elastic/observability-design review in terms of code refactor. Can you seek a review from an engineer within @elastic/observability-ui to review this PR?

@formgeist Yes, certainly.

I will also look into updating the CODEOWNERS file to include either Obs-UI and/or AO so we will have a team that can review code when changes are made to the page as well. Thanks!

@formgeist
Copy link
Contributor

I will also look into updating the CODEOWNERS file to include either Obs-UI and/or AO so we will have a team that can review code when changes are made to the page as well. Thanks!

Sounds good thanks

formgeist
formgeist approved these changes Nov 24, 2022
View reviewed changes
Copy link
Contributor

@formgeist formgeist left a comment

Choose a reason for hiding this comment

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

Design LGTM - I did notice a regression that isn't because of this PR, but something must've changed due to updates to EUI. Wondering if you have time to take a quick look on whether this is an easy fix. The spacing in the footer elements is pretty large, so wonder if there's some messed up flexbox happening.

CleanShot 2022-11-24 at 13 49 25@2x

@CoenWarmer
Copy link
Contributor Author

CoenWarmer commented Nov 24, 2022 •
edited
Loading

Design LGTM - I did notice a regression that isn't because of this PR, but something must've changed due to updates to EUI. Wondering if you have time to take a quick look on whether this is an easy fix. The spacing in the footer elements is pretty large, so wonder if there's some messed up flexbox happening.

@formgeist Good catch! Is indeed due to the changes in Eui. Will include a fix for that.

@CoenWarmer
Copy link
Contributor Author

CoenWarmer commented Nov 24, 2022 •
edited
Loading

@formgeist now fixed!

Screenshot 2022-11-24 at 15 56 03

Also tweaked the HeaderActions a bit so it will look nice on smaller viewports:

Screen.Recording.2022-11-24.at.15.58.05.mov

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 436 439 +3

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.8KB +51.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
osquery 109 115 +6
securitySolution 443 449 +6
total +20

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 68 74 +6
osquery 110 117 +7
securitySolution 520 526 +6
total +21

History

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

kdelemme
kdelemme approved these changes Nov 24, 2022
View reviewed changes
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.

Works fine locally!
Good job with this refactoring 👏🏻 This is a really useful endeavour you're taking on with these refactoring!

🍰 If I may suggest to try to split the changes into smaller commits and keeping the changes together.
For example, the PageHeader is extracted from the main component in one commit, but it does not show the removal from the main page component. Making it difficult to review: is it just an extract or there is also some refactoring?
Same for lifting some variables definition in the component, this could be just one commit so it is easier to review.

That being said, I know it's not always possible to do such small incremental changes so take this comment with a pinch of salt ;)

@CoenWarmer
Copy link
Contributor Author

Thanks @kdelemme! Yeah, I agree it's not always easy to review this type of work. I've taken the first step (splitting up the work in many smaller commits) but it would be even better if the breaking off of components from the main component and the creation of the new file is in one commit. There's still the Alerts page to go after this round of PRs, so I'll do my best to take that approach in that PR.

@CoenWarmer CoenWarmer merged commit dd86c7f into elastic:main Nov 24, 2022
@CoenWarmer CoenWarmer deleted the chore/refactor-overview-page branch November 24, 2022 20:27
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 24, 2022
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 28, 2022
@jloleysens
Merge branch 'main' into files-move-components-to-packages-upload-file
9133d24 
* 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
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kdelemme kdelemme kdelemme approved these changes

@formgeist formgeist formgeist approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor Overview page
5 participants
@CoenWarmer @formgeist @kibana-ci @kdelemme @kibanamachine