Re: [PR] fix(dashboard): keep filter config modal within iframe [superset]

["fitzee (via GitHub)" <gi...@apache.org>]

fitzee commented on code in PR #43632:
URL: https://github.com/apache/superset/pull/43632#discussion_r3901953047


##########
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx:
##########
@@ -359,7 +358,7 @@ export default function DateFilterLabel(props: DateFilterControlProps) {
       defaultOpen={show}
       open={show}
       onOpenChange={toggleOverlay}
-      overlayStyle={{ width: '600px' }}
+      overlayStyle={{ width: 'min(600px, calc(100% - 32px))' }}

Review Comment:
   Agreed. Changed the cap to `min(600px, calc(100vw - 32px))`, so it remains relative to the iframe viewport when the popup is mounted inside **More filters** rather than inheriting that dropdown's width. The regression test now asserts the viewport-relative cap in both `getPopupContainer` modes. Implemented in `6f3a0c8947` and `d151f63eee`.
   



##########
superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx:
##########
@@ -75,9 +73,10 @@ export const BaseModalWrapper = styled(StyledModal)<BaseModalWrapperProps>`
 export const BaseModalBody = styled.div<BaseModalBodyProps>`
   display: flex;
   height: 100%;
-  min-height: 500px;
+  min-height: 0;

Review Comment:
   Confirmed by the independent Chromium reproduction. I removed the percentage-height refactor rather than trying to patch an indefinite chain: `BaseModalBody` is back to its 500px floor, `BaseForm` no longer adds percentage heights, and the existing independently scrollable sidebar/tab constraints are restored. The PR description has been narrowed accordingly; it no longer claims a modal-height repair. Implemented in `6f3a0c8947`.
   



##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/ConfigModalSidebar/ConfigModalSidebar.tsx:
##########
@@ -36,14 +36,12 @@ import { FilterRemoval } from '../types';
 import { FILTER_TYPE, CUSTOMIZATION_TYPE } from '../DraggableFilter';
 import { isFilterId, isChartCustomizationId, isDivider } from '../utils';
 
-// max-height constrains the sidebar so its inner Collapse can scroll when
-// there are many filters (sc-101839). The parent height chain through the
-// antd Form is unreliable, so a viewport-relative max-height is used instead
-// of height: 100%.
 const StyledSidebarFlex = styled(Flex)`
   min-width: 290px;
   max-width: 290px;
-  max-height: 70vh;
+  height: 100%;

Review Comment:
   Agreed. Restored `max-height: 70vh` and the sc-101839 comment verbatim. The parent height chain is not treated as complete, and the PR description explicitly records why the height leg was removed. Implemented in `6f3a0c8947`.
   



##########
superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx:
##########
@@ -99,8 +99,11 @@ test('DateFilter popover should attach to document.body when not overflowing', (
 
   userEvent.click(screen.getByText(NO_TIME_RANGE));
 
-  const popover = document.querySelector('.time-range-popover');
+  const popover = document.querySelector<HTMLElement>('.time-range-popover');
   expect(popover?.parentElement).toBe(document.body);
+  expect(popover).toHaveStyle({

Review Comment:
   Agreed. Added a probe around the underlying Superset `Popover` and a focused assertion that opening `DateFilterLabel` passes `SHIFT_INTO_VIEWPORT` as the resolved `autoAdjustOverflow` value. Re-adding `autoAdjustOverflow={false}` would fail that test. The existing declaration assertion is retained and now covers the `100vw` cap in both popup-container modes. Implemented in `6f3a0c8947` and `d151f63eee`.
   



##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -967,7 +967,7 @@ const FiltersConfigForm = (
   return (
     <Tabs
       allowOverflow={false}
-      contentHeight={`calc(100vh - ${theme.sizeUnit * 55}px)`}
+      fullHeight

Review Comment:
   Confirmed. Removed `fullHeight` and restored the explicit `contentHeight={calc(100vh - ...)}` anchor, preserving the existing `.ant-tabs-body-holder` scrolling behavior in the non-expanded modal. The entire height leg is out of the PR. Implemented in `6f3a0c8947`.
   



-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org