-
Notifications
You must be signed in to change notification settings - Fork 862
feat(EuiPopover): change default offset and disable arrow by default #9086
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
base: main
Are you sure you want to change the base?
Conversation
1ddf661 to
2c97900
Compare
- Update EuiPopover default offset from 0 to 4px for better visual spacing - Disable arrow by default (hasArrow: false) for cleaner popover appearance - Fix EuiTourStep to use popover default instead of hardcoded 0 - Update positioning tests to reflect new default behavior
- Add cross-axis position clamping in getCrossAxisPosition() to prevent popover from being positioned outside container bounds after arrow buffer adjustments - Update early termination logic in findPopoverPosition() to prioritize user's preferred anchorPosition when multiple positions achieve perfect fit (1.0) - Synchronize CSS transform with JavaScript offset by making translateDistance dynamic based on hasArrow prop - Pass offset and hasArrow props to EuiPopoverPanel for accurate transform calculations - Fix useMemo dependencies in EuiPopoverPanel to include offset and hasArrow This fixes an issue where popovers with hasArrow: false would incorrectly reposition to alternative positions (e.g., 'right' instead of 'bottom') even when the preferred position had sufficient space. The root cause was arrow buffer logic pushing the popover slightly outside container bounds (~3px on cross-axis), artificially lowering the fit score from 1.0 to 0.9947 for the preferred position.
Updated Loki reference images to reflect corrected popover positioning behavior. These changes show popovers now correctly respecting the user's preferred anchorPosition when sufficient space is available, rather than incorrectly repositioning to alternative positions.
- Update arrow positioning test expectations to reflect clamping behavior - Update offset test expectations to match new visual offset calculations
10b3587 to
4a42303
Compare
The offset parameter was not used in the CSS styles calculation. The translateDistance only depends on hasArrow to determine if an 8px transform should be applied.
| const euiThemeContext = useEuiTheme(); | ||
| const cssStyles = useMemo(() => { | ||
| const styles = euiPopoverPanelStyles(euiThemeContext); | ||
| const styles = euiPopoverPanelStyles(euiThemeContext, hasArrow); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hasArrow prop synchronizes the CSS transform distance (8px when true, 0px when false) with the JavaScript positioning calculations, ensuring accurate fit scores and preventing incorrect popover repositioning.
| className, | ||
| closePopover, | ||
| anchorPosition = 'downLeft', | ||
| offset = 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very nitpicky, but it somehow looks better when the input-related popovers a just a tiny bit closer. Forgive me :D
| panelPaddingSize: 'm', | ||
| buffer: 16, | ||
| hasArrow: true, | ||
| anchorPosition: 'downCenter', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this to let the story apply the defaults
| anchorPosition: 'downLeft', | ||
| panelPaddingSize: 'm', | ||
| hasArrow: true, | ||
| hasArrow: false, | ||
| offset: 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The heart of the change - new defaults for a more modern look
| button={button as HTMLElement & ReactNode} | ||
| className={anchorClasses} | ||
| anchorPosition={anchorPosition} | ||
| hasArrow={hasBeacon} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choosing to keep the arrow on tour popovers 1) as it points to a specific part of the tour and 2) when a beacon presen, it attaches to/near the arrow space. Removing it would require further refactoring, and the arrow seems still relevant for tours.
|
|
||
| // If we've already found the ideal fit, use that position. | ||
| if (bestFit === 1) { | ||
| // If we've found a perfect fit for the user's preferred position, use it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to stop after the first perfect fit position is found. However, more than one position can be a perfect fit (i.e. has enough room to fit the entire popover). A bug was found that made the likelihood of this happening low (it not impossible) thus why it surfaced here.
The logic starts by checking the bottom position scoring - regardless of what you passed (e.g. rightDown) which meant if bottom scores a fit of 1, then your desired selection may never get tested/applied.
| const visualOffset = this.props.attachToAnchor | ||
| ? offset | ||
| : this.props.hasArrow | ||
| ? 16 + offset - 8 // JavaScript offset minus CSS transform | ||
| : offset; // No penalty for hasArrow: false - let the algorithm choose naturally | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The visualOffset calculation is now in sync with the CSS transform in _popover_panel.styles.ts. When hasArrow: true, we subtract the 8px CSS transform from the total offset so the positioning algorithm uses the actual visual distance.
This fixes incorrect repositioning that occurred when the algorithm thought it needed more space than was visually required.
| // Ensure the popover stays within container bounds after arrow adjustment | ||
| // Get the buffer values to know the actual container bounds | ||
| const [topBuffer, rightBuffer, bottomBuffer, leftBuffer] = | ||
| getBufferValues(buffer); | ||
| const combinedBounds = intersectBoundingBoxes( | ||
| windowBoundingBox, | ||
| containerBoundingBox | ||
| ); | ||
|
|
||
| // Calculate the minimum position (accounting for buffer) | ||
| const minPosition = | ||
| combinedBounds[crossAxisFirstSide] + | ||
| (crossAxisFirstSide === 'top' | ||
| ? topBuffer | ||
| : crossAxisFirstSide === 'right' | ||
| ? rightBuffer | ||
| : crossAxisFirstSide === 'bottom' | ||
| ? bottomBuffer | ||
| : leftBuffer); | ||
|
|
||
| // Clamp the position to not go outside the container bounds | ||
| if (crossAxisPosition < minPosition) { | ||
| crossAxisPosition = minPosition; | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prevent the popover from being positioned outside container bounds after arrow buffer adjustments.
The arrow buffer logic shifts the popover to ensure the arrow has adequate spacing from the popover edges/corner (i.e. so that CSS triangle does not sit on the edge/overlap). However, this could push the popover slightly (~3px to the left in this particular data grid story) outside the container boundaries on the cross-axis, artificially lowering the fit score from 1.0 to 0.9947 for the preferred position.
By ensureing crossAxisPosition stays within the combinedBounds (accounting for buffer values), we ensure accurate fit calculations and prevent the algorithm from incorrectly choosing alternative positions.
In short, this led to bottom fit score being .99 and right being 1 while both had sufficient space for the size of the popover.
- EuiInputPopover: left position now 16px (was 0px) due to cross-axis clamping - EuiDataGrid cell popover: positions updated to 16px left and 75px top due to clamping and new offset calculations - Draggable columns: reordered assertions to wait for popover close before checking focus - Keyboard shortcuts: updated snapshot for popover positioning changes
The keyboard shortcuts popover now uses the new default hasArrow: false, resulting in different positioning and no arrow element in the snapshot.
Reorder to check for popover closure before checking focus in three more tests. This addresses timing issues where focus checks could fail before the popover finishes closing.
The 'should close data grid toolbar popovers' test was failing because the Columns popover now overlaps both the column headers with the new EuiPopover defaults (hasArrow: false, smaller offset).
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
| // Click on the toolbar area (outside the popover) to dismiss it | ||
| cy.get('[data-test-subj="dataGridControls"]').realClick({ | ||
| position: 'center', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With no arrow, the popover covers the header cell which caused the test to fail as the click happens on top of the popover. This changes places the click in the center of the control bar - above the header cells and on the same row as the anchor button - which seems like a more typical user behavior. Clicking on a visible header cell would also work, but this control bar element felt more reliable for testing purposes.
With arrow (before)

Closes #8931
Summary
Primary change
Update
EuiPopoverdefaults for modern popover appearancehasArrow: falseoffset: 4anchorPosition: downLeftSecondary changes
EuiInputPopoverdefaultoffsetto 2px to retain tighter association for inputs (effectively no visual change)EuiTourStepto keep the arrow when beacon is present (effectively no visual change)EuiTourStepstory to use popover default instead of hardcoded valueWhy are we making this change?
Part of a larger effort to modernize our product UIs.
Screenshots
EuiPopover(default 4px offset)

EuiInputPopover(visually unchanged)(default 2px offset)

EuiContextMenu(in Popover; this is an existing storybook)EuiTour(visually unchanged)Impact to users
Minimal - visual only
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
@defaultif default values are missing) and playground toggles