mikebridge commented on PR #43632:
URL: https://github.com/apache/superset/pull/43632#issuecomment-5455330080
Ran a five-lens review of this at `8d53709d`. It confirms every point in
@aminghadersohi's threads and adds a few things — the short version is that the
height leg should come out of this PR rather than be patched, and the PR needs
a real host+iframe reproduction because most of it doesn't touch the reported
symptom.
**Blocker — the default (non-expanded) modal stops scrolling on desktop.**
Amin asked whether the `height: 100%` chain actually resolves; four independent
headless-Chromium replicas of the antd 6 modal chain say it doesn't.
`.ant-modal-container` has `max-height: calc(100vh - 32px)` but no `height`
outside the `expanded` branch, so every percentage below it computes to `auto`
(Flexbox §9.8), the sidebar Collapse and `.ant-tabs-body-holder` grow to
content height, and the `overflow: hidden` on `.ant-modal-body` (and the new
one on `BaseModalBody`) clips with no scrollbar. Numbers at 1280×800 with 40
filters + a 2000px form — before: sidebar client/scroll 518/1640, tab holder
596/2016 (both scroll); after: 1640/1640 and 2016/2016 (neither scrolls,
~1000–1400px unreachable). Same at 700×500, so inside an iframe it only works
after clicking **Expand**. This is the failure the deleted sc-101839 comment in
`ConfigModalSidebar.tsx` was warning about.
Two fixes, both verified in the replica: (a) keep the `vh` anchors
(`max-height: 70vh`, `contentHeight`) — `vh` already resolves against the
iframe's own viewport, so they were never the iframe problem — and drop only
the width change and the 500px floor; or (b) keep the chain and give
`.ant-modal-container` a definite height in the non-expanded branch too. (a) is
smaller and keeps the height refactor bisectable as its own PR.
**High**
- `DateFilterLabel.tsx:361` — as Amin says, `calc(100% - 32px)` resolves
against the "More filters" dropdown on the overflowing-filter-bar leg; measured
at ~216px vs 600px before, on a plain desktop horizontal filter bar.
`min(600px, calc(100vw - 32px))` gives the intended behaviour on both legs,
since an iframe's `vw` is the iframe viewport.
- **The width and height legs are behaviour-neutral inside an iframe.**
`@media (max-width: 912px)`, `vh`, `vw`, antd's own `.ant-modal { max-width:
calc(100vw - 32px) }` and the PR's `100%` of the fixed `.ant-modal-wrap` all
measure the iframe's viewport, so old and new CSS produce identical modal
geometry (880px at 1280, 668px at 700, before and after). SC-111114 describes
the host page's sidebar covering the left of the iframe — occlusion from
*outside* the iframe, which nothing inside it can observe. Could you reproduce
in the customer's actual host+iframe geometry and attach before/after? If the
iframe is wider than its visible region, the remedy is host-side.
- The `autoAdjustOverflow={false}` removal is the one leg that could
plausibly change what the customer sees, and it's global (Explore, vertical
filter bar, config modal — not iframe-scoped) and untested: a control run with
the prop re-added keeps all 6 `DateFilterLabel` tests green.
`ControlPopover.test.tsx:252-288` has the pattern for asserting the resolved
prop; and a line in the description on why the #31973 opt-out is safe to drop
would help.
**Smaller**
- `width: min(…, calc(100% - 32px)) !important` plus `max-width: calc(100% -
32px)` say the same thing (`max-width` always wins), and for `expanded` the
`min()` collapses to `calc(100% - 32px)`; antd already applies the same `vw`
cap. One `width` line + one `max-width` (with a comment that it overrides
antd's `vw`-based cap) reads clearer, and `MIN_WIDTH` no longer means minimum.
- The new `SharedStyles` tests are non-vacuous but assert declaration
strings — they pass while the layout above is broken. The property worth
guarding ("with N filters at 700×500 the sidebar scrolls and the last filter is
reachable, expanded and not") is a Playwright spec.
- `max-height: 100%` beside `height: 100%` is inert; the `toHaveStyle` on
`min(…)` passes via a jsdom/cssstyle serialisation quirk (both sides mangle
identically) — a prop assertion on `ControlPopover` is sturdier.
- Nice: `min-height: 0` on each flex link is the right discipline and should
survive whichever fix lands; using Tabs `fullHeight` over the `calc(100vh -
sizeUnit*55)` magic number is the right direction. The old non-expanded rule
was the unitless `width: 880 !important` (invalid, silently dropped) — worth a
line in the description that this fixes it incidentally.
CI: the CANCELLED/FAILURE entries in the rollup are from the run superseded
at 03:29Z; the re-run is fully green, so no re-run needed.
_Review produced by five AI lenses (react, css, preset, committer,
clean-code) with independent headless-Chromium layout replicas; posted by
@mikebridge after reading. Full report in the spec repo._
--
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]
