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.

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

[kbn-expandable-flyout] - refactor push/overlay to use redux instead of hooks #192745

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Sep 12, 2024

Summary

This PR leverages the redux store work done previously and changes the way we save the state of the flyout type (push vs overlay). Instead of using hooks, this PR is now using redux. In the previous PR we had nested the store information under a new top level store state called panels. This PR introduces a another new top level state called ui. This new state is used to store the pushVsOverlay state, and will be used in the next PR for all the new resized widths logic.

Originally, the pushVsOverlay values were saved for each flyout (using the urlKey). After discussing this (see comments below), the functionality now saves a unique value that will be applied to all expandable flyouts.
Also the value changed by the user is saved in local storage (only if the push vs overlay) option is enabled. The local storage key is expandableFlyout.ui and we store a simple object:

{
  pushVsOverlay: string;
}

This object will see more properties in the next PR that will add all the resizable logic.

Here are the main changes introduced here:

  • introduced the new ui top level slice of state
  • move the push vs overlay logic to redux, hence:
    • create a new uiReducer
    • create a new middleware to push to local storage the values selected by the user
    • delete the old pushVsOverlay hook
    • update the related components to use redux
  • small performance improvement, avoid the SettingsMenu component to render if unnecessary
  • update all the unit tests

None of the changes impact UI and the actual push vs overlay functionality remains unchanged. These are just code improvements!

Checklist

Will help #182526

Sorry, something went wrong.

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0 labels Sep 12, 2024
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner September 12, 2024 16:48
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Left some comments

@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-push-overlay-redux branch from d5df007 to d839661 Compare September 16, 2024 18:58
@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-push-overlay-redux branch from d839661 to 8e091aa Compare September 17, 2024 04:33
@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-push-overlay-redux branch from 8e091aa to f21d1c1 Compare September 17, 2024 05:18
Copy link
Contributor

@christineweng christineweng left a comment

Choose a reason for hiding this comment

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

Overall LGTM! The new set up with redux and local storage looks very nice. Have some nit comments and a question about removing the middleware.

Verified

This commit was signed with the committer’s verified signature.
nibivi77 Benoit Cloutier
…of hooks
@PhilippeOberti PhilippeOberti force-pushed the expandable-flyout-push-overlay-redux branch from f21d1c1 to ec423bd Compare September 17, 2024 18:46
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 994 995 +1
securitySolution 5765 5766 +1
total +2

Async chunks

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

id before after diff
discover 813.0KB 813.7KB +680.0B
securitySolution 20.4MB 20.4MB +3.7KB
total +4.4KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
@kbn/expandable-flyout 2 3 +1

History

  • 💚 Build #234662 succeeded f21d1c1faff09ba97583de52ece163ec8234ac3e
  • 💔 Build #234658 failed 8e091aa08a04e2ec00357417cee519f3aa5351c0
  • 💔 Build #234566 failed d83966107dfcf6618bc14c0acda025cea265250b
  • 💚 Build #233822 succeeded d5df0071d45cceecfde0ce01550e05452300ab72
  • 💔 Build #233816 failed 4ded46f25b2284677d5bfd57647bc7f68dae0e36

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

@PhilippeOberti PhilippeOberti merged commit 31c9e75 into elastic:main Sep 17, 2024
23 checks passed
@PhilippeOberti PhilippeOberti deleted the expandable-flyout-push-overlay-redux branch September 17, 2024 20:15
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 17, 2024
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 17, 2024
…stead of hooks (#192745) (#193225)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[kbn-expandable-flyout] - refactor push/overlay to use redux instead
of hooks (#192745)](#192745)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Philippe
Oberti","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-17T20:15:40Z","message":"[kbn-expandable-flyout]
- refactor push/overlay to use redux instead of hooks
(#192745)","sha":"31c9e75f56fefb18d6ad1e8e53e75c2117c6c343","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Threat
Hunting:Investigations","v8.16.0"],"title":"[kbn-expandable-flyout] -
refactor push/overlay to use redux instead of
hooks","number":192745,"url":"https://github.com/elastic/kibana/pull/192745","mergeCommit":{"message":"[kbn-expandable-flyout]
- refactor push/overlay to use redux instead of hooks
(#192745)","sha":"31c9e75f56fefb18d6ad1e8e53e75c2117c6c343"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192745","number":192745,"mergeCommit":{"message":"[kbn-expandable-flyout]
- refactor push/overlay to use redux instead of hooks
(#192745)","sha":"31c9e75f56fefb18d6ad1e8e53e75c2117c6c343"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Philippe Oberti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants