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

Fixes for EuiHeader z-index, EuiPopover buffer, and EuiCollapsibleNav close button props #3398

Merged
merged 6 commits into from
Apr 29, 2020

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Apr 29, 2020

Continued support for Kibana K8

1. EuiHeader z-index

It was found that the fixed EuiHeader's z-index was too high, causing popovers to slip underneath. While the primary fix would be to contain the popover's within Kibana's main app area, this isn't possible at the moment.

Screen Shot 2020-04-28 at 21 07 00 PM

The quick fix is to lower the header's z-index from 7000 to 1000 (keeping the same z-index for static and fixed position). This allows the popovers to overlap the header (for now).

Screen Shot 2020-04-28 at 21 08 50 PM

However, this caused issues when flyouts or the nav (with EuiOverlayMask) were open, where the nav would then be hidden underneath (including the collapsible nav).

Screen Shot 2020-04-28 at 21 10 21 PM

The fix was to add to the body's class list a class indicating when a flyout is open .euiBody--hasFlyout or when the nav is open .euiBody--collapsibleNavIsOpen and use that to increase the header's z-index up to 8001 (1 more than $euiZModal used by .euiFlyout).

Screen Shot 2020-04-28 at 21 24 55 PM

This does mean that both the nav and a flyout can be at the same plane (visible), but that's hardly a showstopper at the moment.

EuiPopover buffer

The first screenshot also shows how EuiPopover's default buffer (minimum distance between popover and container/browser window) is too small for some places. Dashboard's app padding is a mere 8px while the default buffer is 16px. This PR adds the ability to change the overall buffer via a new prop.

Image 2020-04-28 at 9 17 08 PM

I didn't add a docs example for this prop as it should rarely be used.

EuiCollapsibleNav close button props

It was also asked that we provide more options to pass to the close button, like data-test-subj. So now there is a closeButtonProps prop that extends EuiButtonEmpty. cc @myasonik

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in IE11 and Firefox
  • Props have proper autodocs
  • Added documentation 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 requested review from snide and thompsongl April 29, 2020 01:27
@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor

Dashboard's app padding is a mere 8px while the default buffer is 16px.

Should we change Dashboard's app padding?

cc @ryankeairns

Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

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

🚀 Thank you for the quick turnaround!

@ryankeairns
Copy link
Contributor

Dashboard's app padding is a mere 8px while the default buffer is 16px.

Should we change Dashboard's app padding?

cc @ryankeairns

If it's an outlier, then probably. That said, they are doing a lot of work in that app these days, so it would be good to coordinate with them even on a seemingly minor change. I've been working with that team and can get it in the queue.

@kibanamachine
Copy link

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

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Code LGTM!

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.

LGTM, If we figure out something better, we can always patch.. I don't like the solution of allowing the popovers to sit above the header because it feels a little broken. Given the smallish size of the header, I'd rather see the popovers that could have problems with initial overlap use different anchor positions. In almost any situation they should be pointing down if they are that close to the header. I know this bug is how it currently exists (it wasn't always like this), but it feels broken there too.

We're time constrained though, and as @cchaos mentions, it's not the end of the world. I have a worry that changing the header zindex this much lower will have other ramifications, and we'll start seeing bleeds beyond the popovers.

I don't have time today to jump into the code more heavily to give alternative solutions, so my only suggestion is to make sure there's enough props and className passdowns to make it so this can be tweaked down at the Kibana level should the need arise. A quick scan looks like that's covered.

@cchaos
Copy link
Contributor Author

cchaos commented Apr 29, 2020

Given the smallish size of the header, I'd rather see the popovers that could have problems with initial overlap use different anchor positions

Yep, that would be the ideal solution and hopefully with the added buffer prop to EuiPopover, some of them should be able to get fixed this way.

This is definitely a stop-gap fix for the moment, and we're not introducing more bugs that don't already exist in Kibana. I was mainly needing a gut check that this looks ok and I wasn't doing anything inherently weird or wrong.

@cchaos
Copy link
Contributor Author

cchaos commented Apr 29, 2020

I just linked this PR over to Kibana and seems to be working as expected 🎉

@kibanamachine
Copy link

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

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

Successfully merging this pull request may close these issues.

6 participants