Skip to content

[RAC][Observability] fix flyout in fullscreen mode#108746

Merged
mgiota merged 3 commits intoelastic:masterfrom
mgiota:108174_fullscreen_flyout_fix
Aug 17, 2021
Merged

[RAC][Observability] fix flyout in fullscreen mode#108746
mgiota merged 3 commits intoelastic:masterfrom
mgiota:108174_fullscreen_flyout_fix

Conversation

@mgiota
Copy link
Copy Markdown
Contributor

@mgiota mgiota commented Aug 16, 2021 •
edited
Loading

Fixes #108174

full_screen_fix.mov

cc @katrin-freihofner It was easier to fix than search how to hide the Full screen button that comes with EuiDataGrid

@mgiota mgiota added v7.15.0 v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services labels Aug 16, 2021
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Aug 16, 2021

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@mgiota mgiota added Theme: rac label obsolete auto-backport Deprecated - use backport:version if exact versions are needed labels Aug 16, 2021
@mgiota mgiota self-assigned this Aug 16, 2021
@mgiota mgiota added the release_note:skip Skip the PR/issue when compiling release notes label Aug 16, 2021
@mgiota mgiota mentioned this pull request Aug 16, 2021
Merged
@mgiota
Copy link
Copy Markdown
Contributor Author

mgiota commented Aug 17, 2021

@elasticmachine merge upstream

@katrin-freihofner
Copy link
Copy Markdown

katrin-freihofner commented Aug 17, 2021

@mgiota thanks! When a user has many alerts (full page), the pagination will be at the very bottom, right?

@Kerry350 Kerry350 self-requested a review August 17, 2021 08:54
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 17, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
observability 244 253 +9

Async chunks

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

id before after diff
observability 483.6KB 486.7KB +3.1KB

History

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

cc @mgiota

Kerry350
Kerry350 approved these changes Aug 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Fixes the bug and works as expected 👍

I've left a couple of comments, but I don't think there's anything that can / should be actioned right now. I think we can consider this is a suitable fix for 7.15. As a followup it would be good if the EUI Datagrid could handle this flyout scenario on it's side.

x-pack/plugins/observability/public/pages/alerts/styles.scss
@@ -0,0 +1,12 @@
$fullscreenFlyoutTop: 72px;
Copy link
Copy Markdown
Contributor

@Kerry350 Kerry350 Aug 17, 2021

Choose a reason for hiding this comment

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

This "magic number" makes me a little nervous. There are sass variables for things like the kbnHeaderOffset: https://github.com/elastic/kibana/blob/master/src/core/public/_variables.scss#L6, but there doesn't seem to be anything suitable for here and this use case.

x-pack/plugins/observability/public/pages/alerts/styles.scss
@@ -0,0 +1,12 @@
$fullscreenFlyoutTop: 72px;
.kbnBody.euiBody--headerIsFixed.euiDataGrid__restrictBody {
Copy link
Copy Markdown
Contributor

@Kerry350 Kerry350 Aug 17, 2021

Choose a reason for hiding this comment

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

Again, makes me a little nervous how deeply we're nesting into EUI classes, but I can't think of a better solution right now 🤔

@mgiota mgiota merged commit 8724826 into elastic:master Aug 17, 2021
@kibanamachine kibanamachine mentioned this pull request Aug 17, 2021
Merged
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2021
@mgiota @kibanamachine
[RAC][Observability] fix flyout in fullscreen mode (elastic#108746)
36e93bd 
* [RAC][Observability] fix flyout in fullscreen mode

* eslint fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Aug 17, 2021

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 17, 2021
@kibanamachine @mgiota
[RAC][Observability] fix flyout in fullscreen mode (#108746) (#108871)
0ace9ff 
* [RAC][Observability] fix flyout in fullscreen mode

* eslint fixes

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: mgiota <giota85@gmail.com>
@mgiota mgiota deleted the 108174_fullscreen_flyout_fix branch January 4, 2022 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Kerry350 Kerry350 Kerry350 approved these changes

Assignees

@mgiota mgiota

Labels

auto-backport Deprecated - use backport:version if exact versions are needed 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 Theme: rac label obsolete v7.15.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[RAC][Observability] Alerts table: fullscreen feature does not work as expected

5 participants

@mgiota @elasticmachine @katrin-freihofner @kibanamachine @Kerry350