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 `0a412bdd0f` and `972844d8c9`.
##########
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 `0a412bdd0f`.
##########
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 `0a412bdd0f`.
##########
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
`0a412bdd0f` and `972844d8c9`.
##########
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 `0a412bdd0f`.
--
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]
