aminghadersohi commented on code in PR #44274:
URL: https://github.com/apache/superset/pull/44274#discussion_r4017846141
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx:
##########
@@ -201,9 +201,13 @@ const ColumnSelectPopoverTriggerInner = ({
title={popoverTitle}
destroyOnHidden
>
- {/* Wrap in span so the Popover can attach a ref without relying
- on findDOMNode (deprecated in React 18+). */}
- <span>{children}</span>
+ {/* Wrap in a span so the Popover can attach a ref without relying
+ on findDOMNode (deprecated in React 18+). It must be block-level:
+ antd anchors the popup to this element's box, and a bare inline
+ span around block content (or the empty placeholder the "add new"
+ popovers use) measures 0×0 at the control's left edge, which
+ renders the popover detached (sc-120502). */}
+ <span style={{ display: 'block' }}>{children}</span>
Review Comment:
Measured in Chrome (antd 6.6.2): an inline span around *block* content is
not 0×0 — a 24px-tall chip child reports the full child box, which is why the
chip popovers already work; the collapse needs a **zero-height** child. Same
wording in the other two files.
```suggestion
{/* Wrap in a span so the Popover can attach a ref without relying
on findDOMNode (deprecated in React 18+). It must be block-level:
antd anchors the popup to this element's box, and an inline span
whose child has no height — the empty placeholder the "add new"
popovers pass — collapses to 0×0 at the control's left edge, so
the
popup is placed against a point (sc-120502). */}
<span style={{ display: 'block' }}>{children}</span>
```
##########
superset-frontend/src/explore/components/controls/DndColumnSelectControl/ColumnSelectPopoverTrigger.tsx:
##########
@@ -201,9 +201,13 @@ const ColumnSelectPopoverTriggerInner = ({
title={popoverTitle}
destroyOnHidden
>
- {/* Wrap in span so the Popover can attach a ref without relying
- on findDOMNode (deprecated in React 18+). */}
- <span>{children}</span>
+ {/* Wrap in a span so the Popover can attach a ref without relying
+ on findDOMNode (deprecated in React 18+). It must be block-level:
+ antd anchors the popup to this element's box, and a bare inline
+ span around block content (or the empty placeholder the "add new"
+ popovers use) measures 0×0 at the control's left edge, which
+ renders the popover detached (sc-120502). */}
+ <span style={{ display: 'block' }}>{children}</span>
Review Comment:
Unconverted, same shape: `ContourPopoverTrigger.tsx:57` and
`ColorBreakpointPopoverTrigger.tsx:59` — `ControlPopover`, controlled add-new,
bare inline `<span>` around a zero-height placeholder, measured 0×0 at the
panel's left edge just like here. No fence: the fix is in those files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
