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

[Infra UI] Fix filter popovers not being closed on trigger button click #164060

Conversation

mykolaharmash
Copy link
Contributor

@mykolaharmash mykolaharmash commented Aug 16, 2023

Fixes #96534

Summary

Fixes the bug with some popovers are not closed if their trigger buttons are clicked

Bug demos from the original issue
Inventory:

Metrics Explorer

How to test

  • Checkout the branch locally
  • Goto "Inventory" section
  • Click on "Hosts" filter
  • Make sure popover opens
  • Click on "Hosts" again
  • Make sure the popover closes
  • Check the same for "Sort by" filter
  • Check the same for chart "Actions" in "Metrics Explorer"

@mykolaharmash mykolaharmash requested a review from a team as a code owner August 16, 2023 11:58
@mykolaharmash mykolaharmash added Feature:Metrics UI Metrics UI feature Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes labels Aug 16, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI)

@mykolaharmash mykolaharmash changed the title [Infra UI] Fix filter popovers not being closed on trigger click [Infra UI] Fix filter popovers not being closed on trigger button click Aug 16, 2023
@mykolaharmash mykolaharmash added v8.10.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.11.0 and removed v8.10.0 labels Aug 16, 2023
@mykolaharmash
Copy link
Contributor Author

@elasticmachine merge upstream

@mykolaharmash mykolaharmash marked this pull request as draft August 16, 2023 12:32
@mykolaharmash mykolaharmash force-pushed the 96534-fix-stuck-inventory-filter-popovers branch from 56c3d26 to 5b6bf60 Compare August 16, 2023 13:28
@mykolaharmash mykolaharmash marked this pull request as ready for review August 16, 2023 13:30
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
infra 2.0MB 2.0MB +9.0B

History

  • 💔 Build #150938 failed 2d714c0c63c9fa9a59b6cd8f3839f4c69043d17e

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

@@ -197,6 +197,10 @@ export default ({ getPageObjects, getService }: FtrProviderContext) => {
await pageObjects.infraHome.openTimeline();
await pageObjects.infraHome.closeTimeline();
});

it('toggles the inventory switcher', async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this one test just to get familiar with the testing setup.

Copy link
Contributor

@crespocarlos crespocarlos left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 . Thanks for fixing it and adding the test case. I Tested and the popover closes as expected. Congrats on your first PR!

Comment on lines 33 to +34
const closePopover = useCallback(() => setIsOpen(false), []);
const openPopover = useCallback(() => setIsOpen(true), []);
const togglePopover = useCallback(() => setIsOpen((currentIsOpen) => !currentIsOpen), []);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just FYI, there is a custom hook that could be used here. ex:

 const [isPopoverOpen, { off: closePopover, toggle: togglePopover }] = useBoolean(false);

Mentioning it for the future, if you come across anything that might be interesting to use it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, thank you! 👍

@mykolaharmash mykolaharmash merged commit c8083dd into elastic:main Aug 17, 2023
@mykolaharmash mykolaharmash deleted the 96534-fix-stuck-inventory-filter-popovers branch August 17, 2023 09:05
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2023
…ck (elastic#164060)

Fixes elastic#96534

## Summary

Fixes the bug with some popovers are not closed if their trigger buttons
are clicked

*Bug demos from the original issue*
Inventory:

![](https://user-images.githubusercontent.com/4104278/113987390-a222e900-984e-11eb-872b-f12f5abe4540.gif)
Metrics Explorer

![](https://user-images.githubusercontent.com/4104278/113988929-3b9eca80-9850-11eb-859a-c639b89af8a5.gif)

### How to test

* Checkout the branch locally
* Goto "Inventory" section
* Click on "Hosts" filter
* Make sure popover opens
* Click on "Hosts" again
* Make sure the popover closes
* Check the same for "Sort by" filter
* Check the same for chart "Actions"  in "Metrics Explorer"

(cherry picked from commit c8083dd)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.10

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 Aug 17, 2023
…ton click (#164060) (#164143)

# Backport

This will backport the following commits from `main` to `8.10`:
- [[Infra UI] Fix filter popovers not being closed on trigger button
click (#164060)](#164060)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Mykola
Harmash","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-17T09:05:32Z","message":"[Infra
UI] Fix filter popovers not being closed on trigger button click
(#164060)\n\nFixes #96534 \r\n\r\n## Summary\r\n\r\nFixes the bug with
some popovers are not closed if their trigger buttons\r\nare
clicked\r\n\r\n*Bug demos from the original
issue*\r\nInventory:\r\n\r\n![](https://user-images.githubusercontent.com/4104278/113987390-a222e900-984e-11eb-872b-f12f5abe4540.gif)\r\nMetrics
Explorer\r\n\r\n![](https://user-images.githubusercontent.com/4104278/113988929-3b9eca80-9850-11eb-859a-c639b89af8a5.gif)\r\n\r\n###
How to test\r\n\r\n* Checkout the branch locally\r\n* Goto \"Inventory\"
section\r\n* Click on \"Hosts\" filter\r\n* Make sure popover opens\r\n*
Click on \"Hosts\" again\r\n* Make sure the popover closes\r\n* Check
the same for \"Sort by\" filter\r\n* Check the same for chart
\"Actions\" in \"Metrics
Explorer\"","sha":"c8083dd6767a88f15379531319b5636d20e79451","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Metrics
UI","Team:Infra Monitoring
UI","release_note:skip","backport:prev-minor","v8.11.0"],"number":164060,"url":"https://github.com/elastic/kibana/pull/164060","mergeCommit":{"message":"[Infra
UI] Fix filter popovers not being closed on trigger button click
(#164060)\n\nFixes #96534 \r\n\r\n## Summary\r\n\r\nFixes the bug with
some popovers are not closed if their trigger buttons\r\nare
clicked\r\n\r\n*Bug demos from the original
issue*\r\nInventory:\r\n\r\n![](https://user-images.githubusercontent.com/4104278/113987390-a222e900-984e-11eb-872b-f12f5abe4540.gif)\r\nMetrics
Explorer\r\n\r\n![](https://user-images.githubusercontent.com/4104278/113988929-3b9eca80-9850-11eb-859a-c639b89af8a5.gif)\r\n\r\n###
How to test\r\n\r\n* Checkout the branch locally\r\n* Goto \"Inventory\"
section\r\n* Click on \"Hosts\" filter\r\n* Make sure popover opens\r\n*
Click on \"Hosts\" again\r\n* Make sure the popover closes\r\n* Check
the same for \"Sort by\" filter\r\n* Check the same for chart
\"Actions\" in \"Metrics
Explorer\"","sha":"c8083dd6767a88f15379531319b5636d20e79451"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Mykola Harmash <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Metrics UI Metrics UI feature release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v8.10.0 v8.11.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metrics] Bug: Inventory - The "sort by" control popover gets stuck, no way to dismiss
5 participants