[EuiDataGrid] Fix cell popovers overlapping with modals and flyouts #5461
Conversation
| @@ -41,7 +41,6 @@ export function EuiDataGridCellPopover({ | |||
| panelRef={panelRefFn} | |||
| panelClassName="euiDataGridRowCell__popover" | |||
| panelPaddingSize="s" | |||
| zIndex={8001} | |||
There was a problem hiding this comment.
Not totally sure what the original impetus was for 8001 - if anyone has any context I'd def appreciate it!
There was a problem hiding this comment.
I don't have any extra context here 🤷♂️
| @@ -1,5 +1,6 @@ | |||
| $euiZDataGrid: $euiZHeader - 1; | |||
| $euiZHeaderBelowDataGrid: $euiZHeader - 2; | |||
| $euiZDataGridCellPopover: $euiZHeader; | |||
There was a problem hiding this comment.
Quick note of various z-index levels:
- EuiModal mask overlays are
6000 - EuiFlyout mask overlays are
1000, which is also what$euiZHeaderis - I wanted cell popovers to still be above the base data grid, and being at the same z-index works because a flyout will load in later in the DOM than the popover and take precedence - EuiPopovers have a default inline z-index of
2000, hence the need for!important
There was a problem hiding this comment.
Adding a comment explaining the reasoning to use the $euiZHeader might be helpful in the future.
There was a problem hiding this comment.
👍 Good call!
| $euiZDataGridCellPopover: $euiZHeader; | |
| $euiZDataGridCellPopover: $euiZHeader; // Same z-index as EuiFlyout mask overlays - cell popovers should be under both modal and flyout overlays |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5461/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5461/ |
There was a problem hiding this comment.
Thanks, @constancecchen. LGTM! 🎉
Tested using the following example in Chrome, Safari, Firefox, and Edge and I can confirm that the flyout is now on top of the cell popover:
|
I can't seem to find the flyout example Elizabet showed above, so I'll just mention here to make sure this all works correctly in full screen mode as well. |
|
@cchaos it's an example that I created locally to test. You can find it here: https://gist.github.com/miukimiu/cb1311bd30e9eeef522d42e5bf70cd3c. But I tested in full-screen mode and works well! 👍🏽 The following screenshot was taken in Safari. Full-screen mode with a resized window to make the flyout overlap the cell popover:
|
|
Thanks! 🤔 Maybe we can update one of those buttons to trigger a flyout and one to trigger a modal, so we have a consistent example to test? |
|
I tested in fullscreen mode as well and can confirm that the cell popover works normally there. We do technically have an example that toggles flyouts and modals (the main datagrid kitchen sink example), you just need to use the The full solution for #5310 (allowing consumers to control popover state) should correctly fix/allow consumers to close popovers when modals or flyouts are open, and will definitely get its own example then with buttons that open flyouts/modals. Also @thompsongl did you want to review still or am I OK to merge into main with Elizabet's approval? |
| @@ -41,7 +41,6 @@ export function EuiDataGridCellPopover({ | |||
| panelRef={panelRefFn} | |||
| panelClassName="euiDataGridRowCell__popover" | |||
| panelPaddingSize="s" | |||
| zIndex={8001} | |||
There was a problem hiding this comment.
I don't have any extra context here 🤷♂️
| @@ -1,5 +1,6 @@ | |||
| $euiZDataGrid: $euiZHeader - 1; | |||
| $euiZHeaderBelowDataGrid: $euiZHeader - 2; | |||
| $euiZDataGridCellPopover: $euiZHeader; | |||
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_5461/ |
Summary
Before
(note that the way I get the cell popover to stick around is via the cell action alerts, which is fairly hacky and I wouldn't anticipate it happening in prod)
After
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 any docs examples- [ ] Added or updated jest and cypress tests- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes