aminghadersohi commented on code in PR #43632:
URL: https://github.com/apache/superset/pull/43632#discussion_r3877776973
##########
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:
This drops the only definite height anchor in the non-expanded modal. Every
replacement in this PR (`height: 100%` here and on `BaseForm`,
`height/max-height: 100%` on `StyledSidebarFlex`, `fullHeight` on the config
`Tabs`) is a percentage, and the chain terminates at `.ant-modal-container`,
which has `max-height: calc(100vh - 32px)` but **no** `height`
(`packages/superset-ui-core/src/components/Modal/Modal.tsx:91`).
`BaseModalWrapper` only sets `height: 100%` in the `expanded` branch.
So for the default (non-expanded) modal the ancestor height is `auto`,
percentage heights can resolve to `auto`, and the new `overflow: hidden` on
this element then has no scrolling descendant to hand overflow to. Two cases
worth checking in a browser:
- short content (single filter): the `min-height: 500px` floor is gone, so
the modal can render much shorter than before;
- long filter list: whether the sidebar `Collapse` (`overflow: auto`) still
gets a bounded height, or whether content is clipped by this `overflow: hidden`
with no scrollbar.
The added tests assert CSS declarations via `toHaveStyleRule`, which cannot
observe either outcome, and the PR description notes no local server was
available for an after-screenshot. Given that, a manual pass over
non-expanded/expanded × short/long content before merge would be worth it.
##########
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:
`100%` here is not the viewport. In the `isOverflowingFilterBar` branch just
below, `getPopupContainer` returns `nodeTrigger.parentNode`, which lives inside
the "More filters" `DropdownContainer` popover. `.ant-popover` is `position:
absolute`, so it becomes the containing block for the nested time-range popover
and `calc(100% - 32px)` resolves against that dropdown's content-sized width
(typically well under 600px) rather than against the window.
Net effect: the fix works on the `document.body` leg but shrinks the
time-range editor in the horizontal filter bar overflow menu, which is a
non-iframe path. `calc(100vw - 32px)` gives the same iframe behaviour (an
iframe's `vw` is the iframe viewport) without depending on the popup container:
```suggestion
overlayStyle={{ width: 'min(600px, calc(100vw - 32px))' }}
```
##########
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:
This covers the `overlayStyle` half only. Re-adding
`autoAdjustOverflow={false}` to `ControlPopover` and leaving everything else at
PR head keeps all 6 tests in this file green — so the "restore
`ControlPopover`'s built-in flip/shift behavior" part of the fix, which is the
behavioural change, is unverified.
`ControlPopover` resolves `autoAdjustOverflow ??
getAutoAdjustOverflow(placement)`, and with `placement="right"` that yields
`SHIFT_INTO_VIEWPORT` (`adjustX/adjustY: 1`, `shiftX/shiftY: true`). An
assertion on the resolved `autoAdjustOverflow` reaching the popover would close
the gap.
##########
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:
The comment removed here documented sc-101839 and said explicitly that "the
parent height chain through the antd Form is unreliable, so a viewport-relative
max-height is used instead of `height: 100%`" — which is the exact substitution
being made. If the chain is genuinely complete now, please say so in the PR
description (and ideally replace the comment with one explaining what closed
it), so the next person doesn't re-derive the same conclusion. If it isn't,
this reopens sc-101839. Same question as the `SharedStyles.tsx` thread.
--
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]
