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

[Discover] Dismiss flyouts when opening another one #193865

Merged
merged 6 commits into from
Sep 27, 2024

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Sep 24, 2024

Summary

This PR makes sure that only one flyout is open at a time and automatically dismisses all others.

Checklist

@jughosta jughosta added release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:ES|QL ES|QL related features in Kibana labels Sep 24, 2024
@jughosta jughosta self-assigned this Sep 24, 2024
@jughosta jughosta changed the title 193452 dismiss flyouts [Discover] Dismiss flyouts when opening another one Sep 24, 2024
@jughosta jughosta marked this pull request as ready for review September 24, 2024 17:26
@jughosta jughosta requested review from a team as code owners September 24, 2024 17:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

Copy link
Contributor

@dej611 dej611 left a comment

Choose a reason for hiding this comment

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

While it looks ok as is, I think there are still some minor inconsistencies in discover about fly-outs, in particular with the Inspect and Alert / create rule who do not close the fly-outs underneath.
Perhaps it is arguably a different experience and it still makes sense to keep the fly-outs underneath open there, just worth mentioning it.

From the UX prospective the annoying bit here is the different width of the various fly-outs that makes the layout jump more with this PR changes. While the document flyout is a resizable panel, the other two have fixed width which would be nice to align to avoid too many jumps.

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

ES|QL changes LGTM. (just a data-test-subj).

Maybe we should create an issue about the flyout manager that we were discussing cc @davismcphee. From my understanding this would be the fast fix till we do something better.

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Code changes look good and it works as intended, thanks!

While the document flyout is a resizable panel, the other two have fixed width which would be nice to align to avoid too many jumps.

Maybe the solution is to just make them all resizable? 🙂

Maybe we should create an issue about the flyout manager that we were discussing cc @davismcphee. From my understanding this would be the fast fix till we do something better.

It is and I agree, I created an issue for it here: #194239.

@jughosta jughosta enabled auto-merge (squash) September 27, 2024 06:11
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
canvas 1291 1292 +1
cloudSecurityPosture 651 652 +1
discover 997 998 +1
esqlDataGrid 363 364 +1
eventAnnotationListing 578 579 +1
lens 1461 1462 +1
logsExplorer 562 563 +1
observability 1058 1059 +1
searchPlayground 241 242 +1
securitySolution 5829 5830 +1
slo 844 845 +1
unifiedDocViewer 219 220 +1
unifiedHistogram 144 186 +42
total +54

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/discover-utils 140 146 +6

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.9KB 815.4KB +1.5KB
esql 178.4KB 178.5KB +49.0B
unifiedHistogram 67.2KB 67.9KB +640.0B
unifiedSearch 347.2KB 347.4KB +289.0B
total +2.4KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
unifiedSearch 38.5KB 38.6KB +70.0B
Unknown metric groups

API count

id before after diff
@kbn/discover-utils 174 180 +6

History

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

cc @jughosta

@jughosta jughosta merged commit 6fc017a into elastic:main Sep 27, 2024
20 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11066513519

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 27, 2024
- Closes elastic#193452

## Summary

This PR makes sure that only one flyout is open at a time and
automatically dismisses all others.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

(cherry picked from commit 6fc017a)
@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 27, 2024
…194256)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Dismiss flyouts when opening another one
(#193865)](#193865)

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

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

<!--BACKPORT [{"author":{"name":"Julia
Rechkunova","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-27T07:35:31Z","message":"[Discover]
Dismiss flyouts when opening another one (#193865)\n\n- Closes
https://github.com/elastic/kibana/issues/193452\r\n\r\n##
Summary\r\n\r\nThis PR makes sure that only one flyout is open at a time
and\r\nautomatically dismisses all others.\r\n\r\n\r\n###
Checklist\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"6fc017a597fc34158313b8537f6b6a2536833cba","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:DataDiscovery","backport:prev-minor","Feature:ES|QL"],"title":"[Discover]
Dismiss flyouts when opening another
one","number":193865,"url":"https://github.com/elastic/kibana/pull/193865","mergeCommit":{"message":"[Discover]
Dismiss flyouts when opening another one (#193865)\n\n- Closes
https://github.com/elastic/kibana/issues/193452\r\n\r\n##
Summary\r\n\r\nThis PR makes sure that only one flyout is open at a time
and\r\nautomatically dismisses all others.\r\n\r\n\r\n###
Checklist\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"6fc017a597fc34158313b8537f6b6a2536833cba"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193865","number":193865,"mergeCommit":{"message":"[Discover]
Dismiss flyouts when opening another one (#193865)\n\n- Closes
https://github.com/elastic/kibana/issues/193452\r\n\r\n##
Summary\r\n\r\nThis PR makes sure that only one flyout is open at a time
and\r\nautomatically dismisses all others.\r\n\r\n\r\n###
Checklist\r\n\r\n\r\n- [x] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common scenarios\r\n- [x] This was
checked for
[cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"6fc017a597fc34158313b8537f6b6a2536833cba"}}]}]
BACKPORT-->

Co-authored-by: Julia Rechkunova <[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:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Improve flyout interaction
7 participants