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] Fix visibility of saved search grids when one of them is in fullscreen mode #136198

Merged
merged 5 commits into from
Jul 20, 2022

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Jul 12, 2022

Closes #134032

Summary

This PR adds CSS which overrides Dashboard panel styles when one of grids is in fullscreen mode.

Steps to test:

  1. Create a dashboard
  2. Add multiple Saved Searches to it (next to each other)
  3. Try fullscreen mode for any of them => only the expanded grid should be visible in its fullscreen mode

Checklist

@jughosta jughosta added Feature:Dashboard Dashboard related features release_note:fix backport:skip This commit does not require backporting Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) labels Jul 12, 2022
@jughosta jughosta self-assigned this Jul 12, 2022
@jughosta jughosta added backport:prev-minor Backport to the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Jul 12, 2022
* It can be removed if we don't use inline styles (z-index, position) for Dashboard panels (react-grid-item)
*/
.euiDataGrid__restrictBody .dshDashboardGrid__item {
position: static !important; /* 1 */
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@constancecchen Your suggestion works great for desktop view, thanks!

But there is something weird with mobile view. Although it has position: static !important

the issue with overlapping grids is still present. What is different for mobile view in grids implementation?

Comment on lines 50 to 51
.euiDataGrid__restrictBody .dshDashboardGrid__item {
position: static !important; /* 1 */
Copy link
Member

Choose a reason for hiding this comment

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

It looks like mobile / small screens already has a position: static !important set, interestingly enough. It's not 100% clear to me why it doesn't 'just work' in that case, but as I was tinkering around I found a z-index unset that appears to work for both mobile and desktop:

Suggested change
.euiDataGrid__restrictBody .dshDashboardGrid__item {
position: static !important; /* 1 */
.euiDataGrid__restrictBody .embPanel .embPanel__content {
z-index: unset !important; /* 1 */

Unsetting z-index is probably a safer bet than position in any case. Let me know if that works for you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@constancecchen Yes, this works great for desktop and mobile! Thank you very much for solving the issue 🎉

@jughosta jughosta changed the title [Dashboard][Saved Search] Fix visibility of saved search grids when one of them is in fullscreen mode [Discover] Fix visibility of saved search grids when one of them is in fullscreen mode Jul 13, 2022
@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@jughosta jughosta marked this pull request as ready for review July 13, 2022 09:16
@jughosta jughosta requested review from a team as code owners July 13, 2022 09:16
@elasticmachine
Copy link
Contributor

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

@kibanamachine kibanamachine added the Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas label Jul 13, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@jughosta jughosta added loe:small Small Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Jul 13, 2022
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.

Tested locally and it worked on both desktop and mobile emulation. We could add a functional test for this but may be more effort than it's worth so LGTM!

@jughosta
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 590 595 +5

Async chunks

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

id before after diff
discover 492.5KB 493.2KB +746.0B

History

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

cc @jughosta

Copy link
Contributor

@andreadelrio andreadelrio left a comment

Choose a reason for hiding this comment

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

Sass change LGTM.

@kertal kertal merged commit 42f2bc2 into elastic:main Jul 20, 2022
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.3 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 136198

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jul 21, 2022
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 136198 locally

1 similar comment
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 136198 locally

@kertal
Copy link
Member

kertal commented Jul 22, 2022

💚 All backports created successfully

Status Branch Result
8.3

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

Questions ?

Please refer to the Backport tool documentation

kertal pushed a commit to kertal/kibana that referenced this pull request Jul 22, 2022
…n fullscreen mode (elastic#136198)

(cherry picked from commit 42f2bc2)

# Conflicts:
#	src/plugins/discover/public/embeddable/saved_search_grid.tsx
kertal added a commit that referenced this pull request Jul 22, 2022
…n fullscreen mode (#136198) (#136930)

(cherry picked from commit 42f2bc2)

# Conflicts:
#	src/plugins/discover/public/embeddable/saved_search_grid.tsx

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
@kibanamachine kibanamachine added v8.3.3 and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jul 22, 2022
@jughosta jughosta deleted the 134032-fullscreen-view-on-dashboard branch August 1, 2022 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:small Small Level of Effort release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.3.3 v8.4.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data Tables in dashboard views stay visible when another table is full screened
8 participants