Add zIndexAdjustment prop to EuiPopover #1097
Conversation
| @@ -515,7 +515,7 @@ export function intersectBoundingBoxes(firstBox, secondBox) { | |||
| * relative to the `target` element; if no z-index is defined, returns "0" | |||
| * @param element {HTMLElement|React.Component} | |||
| * @param cousin {HTMLElement|React.Component} | |||
| * @returns {string} | |||
| * @returns {number} | |||
There was a problem hiding this comment.
Do you want to count this as a breaking change?
There was a problem hiding this comment.
This function is only internal.
There was a problem hiding this comment.
I hear ya, just thought I would ask because technically consumers can access it like this:
import { getElementZIndex } from '@elastic/eui/lib/services/popover/popover_positioning';Kibana is accessing the contents of other modules like this, which I'm not a fan of, but I'm not sure if our docs or conventions are clear about how to distinguish public from internal services. No big deal, just thought I'd mention it.
There was a problem hiding this comment.
Ughhh; true. In response to Caroline's request I've reverted the changes to this function, but you're right with the deep-importing that could happen this would be a breaking change.
src/components/popover/popover.js
Outdated
| @@ -436,11 +437,14 @@ EuiPopover.propTypes = { | |||
| ]), | |||
| /** When `true`, the popover's position is re-calculated when the user scrolls, this supports having fixed-position popover anchors. */ | |||
| repositionOnScroll: PropTypes.bool, | |||
| /** Popover content inherits the z-index of the anchor component, zIndexAdjustment is added to this z-index */ | |||
| zIndexAdjustment: PropTypes.number, | |||
There was a problem hiding this comment.
I don't feel strongly about this but I think I've seen "offset" used more often to indicate a value that's added to an original value, e.g. offsetX. Just tossing it out there if you feel it's more descriptive. I'm OK with "adjustment" too as long as it's used consistently for similar variable names elsewhere.
|
What's the thought on doing it this way instead of just allowing a supplied z-index value? I feel like the latter would allow for the most customization. |
|
@cchaos This way the values in JS are not completely tied to the SCSS ones. However, if you think hardcoding z-index in these cases instead of applying a relative value is better/easier I'll make the change! |
|
Yeah my feeling is that, and as it was in my case, that in order to bump the z-index above other elements (not the trigger) I would have to know both the trigger z-index and the other element z-index and then do some math to come up with what would be the difference (adjustment). However, if we just allow a custom z-index, all I have to find is the z-index of the competing element and add however much to that. |
|
@cjcenizal @cchaos changed from adding an offset to being a complete override |
|
Confirmed that it works (in IE as well) before
after
👍 |
|
@chandlerprall I think this one is good to merge |
Closes #1096
Adds new prop to
EuiPopoverallowing its z-index value to be adjusted.