aminghadersohi commented on code in PR #43836:
URL: https://github.com/apache/superset/pull/43836#discussion_r3931345484
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -165,19 +165,48 @@ const StyledContent = styled.div<{
}>`
grid-column: 2;
grid-row: 2;
- /* @z-index-above-dashboard-header (100) + 1 = 101 */
- ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}
+ /* @z-index-above-dashboard-header (100) + 2 = 102: a maximized chart
+ must also cover the version-history overlay (101) so the two stack the
+ same way on both sides of the overlay breakpoint. */
+ ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 102;`}
`;
// Sticks alongside the page scroll so the panel stays fully visible.
+// Below the XXL breakpoint the dashboard grid's min-content width plus the
+// panel exceed the viewport (the content column cannot shrink), which would
+// push the panel past the page's right edge and clip its own controls
+// (sc-119737). Mirror the Explore panel host in spirit — Explore anchors
+// absolutely inside its relatively-positioned container, but the dashboard
+// page owns the scroll, so this pins to the viewport instead. While open at
+// these widths the overlay covers the page's right edge (including the top
+// navbar while scrolled to the top) — accepted: it is a closable surface.
const VersionHistoryColumn = styled.div`
- grid-column: 3;
- grid-row: 1 / span 2;
- position: sticky;
- top: 0;
- align-self: start;
- height: 100vh;
- z-index: 99;
+ ${({ theme }) => css`
+ grid-column: 3;
+ grid-row: 1 / span 2;
+ position: sticky;
+ top: 0;
+ align-self: start;
+ height: 100vh;
+ z-index: 99;
+ @media (max-width: ${theme.screenXLMax}px) {
Review Comment:
Question about the chosen boundary, not an objection.
`theme.screenXLMax` exists (`packages/superset-core/src/theme/types.ts:478`)
and resolves to antd's `screenXXL - 1` = **1599px**. But the commit message
measures the actual overflow as starting "below roughly 1446px", and the
capture you posted is at 1280px.
So between roughly **1447px and 1599px** the side-by-side layout still fit,
and this PR converts it to an overlay anyway — trading a working two-column
layout for the navbar occlusion you documented, in a band where there was no
bug. That may well be deliberate (a standard token beats a magic number derived
from one dashboard's min-content width, and 1446 will drift with filter-bar
width and panel width anyway), and `ExploreVersionHistory`'s `PanelHost` uses
`screenXL` for the same reason.
Just confirming it's a conscious choice rather than "XXL was the nearest
token" — and if it is, a half-sentence in the comment above saying so ("the
measured threshold is ~1446px and moves with filter-bar width; snap to the
breakpoint") would save the next reader the same derivation.
##########
superset-frontend/src/features/versionHistory/DashboardVersionHistory.test.tsx:
##########
@@ -253,3 +253,21 @@ test('withholds restore on an externally managed
dashboard', () => {
expect.objectContaining({ canRestore: false }),
);
});
+
+test('renders nothing in place while the panel is closed', () => {
+ // The DashboardBuilder overlay relies on this contract: the closed
+ // column must stay DOM-empty (the restore modal portals out of it), or
+ // the :empty shadow guard stops matching and a stray shadow line appears
+ // at the viewport edge below the overlay breakpoint (sc-119737).
+ const store = makeTestStore({
+ versionHistory: versionHistoryState({ isPanelOpen: false }),
+ dashboardInfo: {
+ uuid: 'dash-uuid',
+ last_modified_time: 100,
+ dash_edit_perm: true,
+ },
+ dashboardState: { hasUnsavedChanges: false, lastModifiedTime: 500 },
+ });
+ const { container } = renderAdapter(store);
+ expect(container).toBeEmptyDOMElement();
Review Comment:
**The assertion is insensitive to the state it claims to pin — it passes
identically with the panel open.**
Both things that could put DOM into the closed column are mocked away in
this file:
* `jest.mock('./VersionHistoryPanel', ...)` (L26–32) replaces the panel with
`() => null`;
* `jest.mock('./useVersionActions', ...)` (L36–41) returns `restoreModal:
null`.
So `DashboardVersionHistory` renders an empty container in *both* branches
of `if (!isPanelOpen) return restoreModal;`, and `toBeEmptyDOMElement()` cannot
distinguish them.
Measured (RAN, not reasoned): changing L263 to `isPanelOpen: true` and
re-running this test leaves it green —
```
✓ renders nothing in place while the panel is closed (3 ms)
Tests: 7 skipped, 1 passed, 8 total
```
That matters because two comments — L258–261 here and the `&:empty` block at
`DashboardBuilder.tsx:200–204` ("Pinned by the closed-state test in
DashboardVersionHistory.test.tsx") — assert this test guards the sc-119737
shadow sliver. As written it would stay green if the closed path started
rendering the real panel, or if `RestoreConfirmModal` stopped portalling and
rendered in place.
To be clear about the production behaviour: I traced it and **the contract
does hold** — `restoreModal` is always a `<RestoreConfirmModal>` element, but
it returns `null` while `target` is null, and antd's `Modal` portals to
`document.body` when it isn't. It's only the *test* that doesn't prove it.
Two cheap strengthenings, either of which makes it fail on a real regression:
```ts
// closed => the panel component is never rendered at all
expect(mockPanelProps).not.toHaveBeenCalled();
```
and, to cover the modal half, render this case with a non-null
`restoreModal` (e.g. `jest.mocked(useVersionActions)` returning the real
`<RestoreConfirmModal target={null} … />`, or unmocking `useVersionActions` for
this one test) and assert the container is still empty.
bito flagged the `restoreModal` half of this above; the
`VersionHistoryPanel` mock is the larger half, since it's what makes the open
state indistinguishable too.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -165,19 +165,48 @@ const StyledContent = styled.div<{
}>`
grid-column: 2;
grid-row: 2;
- /* @z-index-above-dashboard-header (100) + 1 = 101 */
- ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}
+ /* @z-index-above-dashboard-header (100) + 2 = 102: a maximized chart
+ must also cover the version-history overlay (101) so the two stack the
+ same way on both sides of the overlay breakpoint. */
+ ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 102;`}
Review Comment:
I enumerated everything in the 95–199 band under `superset-frontend/src` and
`packages/` to check the ladder, and **the 101 → 102 bump is correct — nothing
was jumped over**:
| z-index | what |
|---|---|
| 99 | `StyledHeader` (`DashboardBuilder.tsx:128`), `Loading`
(`packages/…/Loading/index.tsx:37`), `VersionHistoryColumn` at ≥XXL |
| 100 | `TabsRenderer:100`, FilterBar `ActionButtons:55`,
`RangeFilterPlugin:74` |
| 101 | `ResizableSidebar` resizer (`:34`), `AddSliceDragPreview` (`:45`),
**VersionHistoryColumn overlay (new)** |
| 102 | **maximized chart (this line)** |
Two notes:
1. **Portalled antd popups are safe.** `DashboardWrapper`'s `StyledDiv` is
`position: relative` with no `z-index`, so it creates no stacking context and
all of the above live in the root one. antd dropdowns/tooltips/popovers portal
to `<body>` at `zIndexPopupBase` (1000+), so a kebab menu opened from inside
the panel still paints above the 101 overlay. No occlusion.
2. **One new tie, cosmetic:** `AddSliceDragPreview` is `position: fixed;
zIndex: 101` and now ties the overlay. Equal z-index resolves by tree order,
and `VersionHistoryColumn` comes after `StyledContent`, so below XXL the "add
chart" drag preview would slide *under* the open panel — where previously
(column at 99) it slid over it. That needs edit mode + panel open + <1600px +
dragging across the panel's strip, and the panel is opaque there anyway, so I'd
leave it; noting it only because you're touching this ladder.
##########
superset-frontend/src/dashboard/components/DashboardBuilder/DashboardBuilder.tsx:
##########
@@ -165,19 +165,48 @@ const StyledContent = styled.div<{
}>`
grid-column: 2;
grid-row: 2;
- /* @z-index-above-dashboard-header (100) + 1 = 101 */
- ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 101;`}
+ /* @z-index-above-dashboard-header (100) + 2 = 102: a maximized chart
+ must also cover the version-history overlay (101) so the two stack the
+ same way on both sides of the overlay breakpoint. */
+ ${({ fullSizeChartId }) => fullSizeChartId && `z-index: 102;`}
`;
// Sticks alongside the page scroll so the panel stays fully visible.
+// Below the XXL breakpoint the dashboard grid's min-content width plus the
+// panel exceed the viewport (the content column cannot shrink), which would
+// push the panel past the page's right edge and clip its own controls
+// (sc-119737). Mirror the Explore panel host in spirit — Explore anchors
+// absolutely inside its relatively-positioned container, but the dashboard
+// page owns the scroll, so this pins to the viewport instead. While open at
+// these widths the overlay covers the page's right edge (including the top
+// navbar while scrolled to the top) — accepted: it is a closable surface.
const VersionHistoryColumn = styled.div`
- grid-column: 3;
- grid-row: 1 / span 2;
- position: sticky;
- top: 0;
- align-self: start;
- height: 100vh;
- z-index: 99;
+ ${({ theme }) => css`
+ grid-column: 3;
+ grid-row: 1 / span 2;
+ position: sticky;
+ top: 0;
+ align-self: start;
+ height: 100vh;
+ z-index: 99;
+ @media (max-width: ${theme.screenXLMax}px) {
+ /* @z-index-above-dashboard-header (100) + 1 = 101 */
+ position: fixed;
+ right: 0;
+ bottom: 0;
+ height: auto;
Review Comment:
**Keep `height: auto` — bito's "Overlay scroll never engages" suggestion
above is incorrect, and switching to `height: 100vh` would be a (harmless but
unnecessary) change made for a wrong reason.**
bito's claim is that with an `auto`-height parent, `Panel { height: 100% }`
resolves to `auto`, so `Body { overflow-y: auto }` never scrolls. That skips
the absolute-positioning height algorithm:
* This box is `position: fixed` with **both** `top: 0` and `bottom: 0` and
`height: auto`, so CSS 2.1 §10.6.4 *solves for* `height` — the used height
becomes definite (the viewport height), not content-derived.
* CSS 2.1 §10.5's "percentage height computes to `auto`" fallback only
applies when the containing block's height depends on its content. Here it
doesn't, so `Panel { height: 100%; }` resolves against the viewport height and
`Body { overflow-y: auto; }` engages normally.
Net effect is identical to `height: 100vh`, and `top`/`bottom` is arguably
the better spelling since it doesn't inherit `100vh`'s mobile-URL-bar
behaviour. This is spec reasoning, not a browser measurement — I can't run a
browser here — but the `top: 0` you inherit from the base rule is doing real
work and is worth keeping as-is.
--
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]
