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

[euiHeaderAffordForFixed] Don't apply offset styles to EuiOverlayMask or EuiFlyout when EuiDataGrid is full screen #5054

Conversation

MichaelMarcialis
Copy link
Contributor

@MichaelMarcialis MichaelMarcialis commented Aug 19, 2021

Summary

It was recently noticed in Kibana that EuiOverlayMask and EuiFlyout components were still accounting for the presence of EuiHeader components, even when EuiDataGrid is in full screen mode. This was due to the EUI mixin euiHeaderAffordForFixed.

This PR changes the mixin to now avoid applying these offset styles (top, height) when EuiDataGrid is in full screen (via the :not(.euiDataGrid__restrictBody) selector).

Additionally, while adding examples of EuiModal and EuiFlyout being triggered from within an EuiDataGrid (per @cchaos 's suggestion), it was discovered that the EuiModal and EuiFlyout components had a z-index that caused them to appear below EuiDataGrid components when in full screen mode (outside Kibana). A fix for this is also included in this PR (thanks to @cchaos for the direction).

Closes elastic/kibana#108681.
Closes elastic/kibana#108896.

CCing @snide, @ryankeairns, @XavierM.

Before:

image

After:

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • [ ] Props have proper autodocs and playground toggles
  • [ ] Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • [ ] Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@cchaos cchaos self-requested a review August 19, 2021 15:08
@MichaelMarcialis MichaelMarcialis marked this pull request as ready for review August 19, 2021 15:18
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5054/

@cchaos
Copy link
Contributor

cchaos commented Aug 19, 2021

💟 Thanks for the PR @MichaelMarcialis !!

I'm wondering if we should also specifically add in some examples to our docs that trigger flyouts and modals so we might better keep track of those things. Would you be up for doing that?

@MichaelMarcialis
Copy link
Contributor Author

MichaelMarcialis commented Aug 19, 2021

I'm wondering if we should also specifically add in some examples to our docs that trigger flyouts and modals so we might better keep track of those things. Would you be up for doing that?

@cchaos: Sure, I can see about adding an example of an EuiDataGrid with flyout and modal triggers. Do you think this new example would make sense as part of the "Data grid styling and control" page? Also, should it be a separate PR or part of this one?

@cchaos
Copy link
Contributor

cchaos commented Aug 19, 2021

You can build it directly into this PR. It'll also make it easier to test this PR to have an example to look at.

How about just updating the action items in the first/initial example which has kind of become our Kitchen sink of sorts.

Screen Shot 2021-08-19 at 13 28 39 PM

If you need help with content, just shout.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5054/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5054/

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

Fantastic work. Thank you for taking this so quickly. I left some small suggestions, but I think it's OK to merge after addressing them.

@chandlerprall @thompsongl I'd like to backport this one as a patch because it's causing breaks in Kibana. Might be a good opportunity to show @breehall and @constancecchen how we do those. I'll make an issue for an upcoming patch.

src/global_styling/variables/_z_index.scss Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Aug 20, 2021

I think this one is worth testing a local build with Kibana to make sure this solves the problem. I'm going to spin that up so it'll be a couple hours before I review 😬

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5054/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

I love the docs info you put into the modal and flyout examples! 💯 Checked in FF, Safari, and Chrome. I checked this in Kibana too, looks like it's working!

Screen Shot 2021-08-20 at 11 07 16 AM

But it looks like we'll need to adjust the full screen data grid placement when there are banners in Kibana.

Screen Shot 2021-08-20 at 11 09 13 AM

@MichaelMarcialis
Copy link
Contributor Author

But it looks like we'll need to adjust the full screen data grid placement when there are banners in Kibana.

Interesting. Looks like it's a z-indexing issue with the banner showing above the data grid. I'll merge this in and take a peek at that after, as that's likely on the Kibana side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants