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

Make EuiPopover copy the anchor's z-index to the popover content #967

Merged
merged 5 commits into from
Jul 3, 2018

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Jul 3, 2018

Fixes #965

This sets the content's z-index from the anchor element's z-index. The value must be calculated/copied instead of hard-coding in css as e.g. the popover may need to show behind a flyout from one button but be in front if it relates to content in the flyout.

without setting z-index
without z-index

with z-index
with z-index

adding
@cchaos sanity check, testing
@nreese / @cjcenizal to make sure the code change & comments are understandable

@nreese
Copy link
Contributor

nreese commented Jul 3, 2018

Can you add the flyout example in your GIF to this PR?

@@ -189,9 +189,15 @@ export class EuiPopover extends Component {
}
});

// the popver's z-index must inherit from the button
// this keeps a button's popver under a flyover that would covert the button
Copy link
Contributor

Choose a reason for hiding this comment

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

spelling "covert" => "cover"

Copy link
Contributor

Choose a reason for hiding this comment

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

popver => popover

Copy link
Contributor

Choose a reason for hiding this comment

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

flyover => flyout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, thanks!

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.

Works in IE too! ;)

@chandlerprall
Copy link
Contributor Author

@nreese I've pushed an update adding a popover to the complicated flyout example

@@ -156,6 +162,13 @@ export class FlyoutComplicated extends Component {
<EuiSpacer size="s" />
<EuiText color="subdued">
<p>Put navigation items in the header, and cross tab actions in a footer.</p>
<EuiPopover
Copy link
Contributor

Choose a reason for hiding this comment

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

@chandlerprall Can you actually move this popover to the flyout body and outside of any EuiText elements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, can you move it out of EuiFlyoutHeader and into <EuiFlyoutBody> below the EuiCodeBlock (line 181)? I want to be sure that when consumers copy-paste this, they're not overusing popovers in the headers.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm
code review and ran flyout examle with popover in Chrome

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM! Nice comments!

@chandlerprall
Copy link
Contributor Author

@cchaos popover has been moved into the flyout body

@cchaos
Copy link
Contributor

cchaos commented Jul 3, 2018

TY!

@chandlerprall chandlerprall merged commit 4d56c9c into elastic:master Jul 3, 2018
@chandlerprall chandlerprall deleted the bug/965-flyout-popover branch July 3, 2018 20:36
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.

4 participants