AmoghAtreya commented on code in PR #44959:
URL: https://github.com/apache/superset/pull/44959#discussion_r4179009752
##########
superset-frontend/src/explore/components/controls/ControlPopover/ControlPopover.tsx:
##########
@@ -84,15 +84,17 @@ const ControlPopover: FC<PopoverProps> = ({
getPopupContainer,
getVisibilityRatio = getElementVisibilityRatio,
open: visibleProp,
+ defaultOpen,
+ onOpenChange,
destroyOnHidden = false,
placement: initialPlacement = 'right',
autoAdjustOverflow,
...props
}) => {
+ const isControlled = visibleProp !== undefined;
const triggerElementRef = useRef<HTMLElement>();
- const [visible, setVisible] = useState(
- visibleProp === undefined ? props.defaultOpen : visibleProp,
- );
+ const [uncontrolledVisible, setUncontrolledVisible] =
useState(!!defaultOpen);
Review Comment:
Regarding bito's message to the codeant assessment, I believe it still isn’t
correct — no change needed.
The original note was about visibility (`uncontrolledVisible`), not
placement. Those are two different hooks on purpose:
- Placement uses `React.useState`, which `setupTest` spies on so we can
assert `'rightTop'` /`'left'` / `'rightBottom'`.
- Open/close uses the named `useState` import, which is not mocked.
Uncontrolled click-to-open, scroll lock, and Escape already pass through
that real setter. If visibility were frozen, those tests would stay closed;
they don’t. `setupTest` is in `ControlPopover.test.tsx` in this PR.
--
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]
