EnxDev commented on code in PR #44359:
URL: https://github.com/apache/superset/pull/44359#discussion_r4087731785
##########
superset-frontend/src/dashboard/components/gridComponents/Row/Row.test.tsx:
##########
@@ -269,6 +278,31 @@ test('should increment the depth of its children', () => {
);
});
+test('row droptarget height tracks the row instead of staying pinned to a
stale measured height (regression for #37644)', () => {
+ const { container, rerender } = setup({ editMode: true });
+ const getDroptargetHeights = () =>
+ Array.from(
+ container.querySelectorAll<HTMLElement>('.empty-droptarget--vertical'),
+ ).map(el => el.style.height);
+
+ // The leading droptarget (index 0) is absolutely positioned, so a
+ // percentage height resolves fine and stretches it to the row via CSS
+ // rather than a pixel value measured from the row's tallest chart -- it
+ // can never be left pinned to a chart's prior (larger) height after a
+ // resize. The droptarget after the chart is an in-flow flex item under
+ // GridRow's indefinite `height: fit-content`, where a percentage height
+ // resolves to `auto`/is ignored per the flexbox spec (defeating the
+ // `align-self: stretch` CSS already declares for it) -- it gets an
+ // explicit `auto` instead, letting that stretch actually apply.
+ expect(getDroptargetHeights()).toEqual(['100%', 'auto']);
+
+ // Something (e.g. hovering the row's own HoverMenu while resizing)
+ // causes Row to re-render after the tallest chart in the row shrinks.
+ rerender(<Row {...props} editMode component={{ ...props.component }} />);
+
+ expect(getDroptargetHeights()).toEqual(['100%', 'auto']);
Review Comment:
The rerender passes the same props and jsdom has no layout, so nothing
shrinks here and this expect can't fail unless the one above already did.
The test also pins inline style strings, so it stays green if `align-self:
stretch` comes out of `GridRow`, and that's the line doing the real work. I'd
drop the rerender half and the "reproduces the feedback loop" line in the
description. A real guard for the layout needs a browser, e.g. a Playwright
check on the droptarget height after shrinking a chart.
##########
superset-frontend/src/dashboard/components/menu/HoverMenu.tsx:
##########
@@ -32,7 +34,7 @@ const HoverStyleOverrides = styled.div`
.hover-menu {
opacity: 0;
position: absolute;
- z-index: 11; /* one more than DragDroppable */
+ z-index: ${HOVER_MENU_Z_INDEX};
Review Comment:
For the resize part I'd fix it here rather than on the handles: this menu
still hit-tests at `opacity: 0`, and that invisible box is what covers the
bottom edge of the chart above.
`pointer-events: none` here, plus `pointer-events: auto` next to the
`opacity: 1` reveal in `DashboardWrapper.tsx:98`, lets the edge through without
touching the `translate3d` stacking contexts. The catch is that a column's menu
only becomes clickable once the pointer has entered the column, so worth
checking those buttons in a browser.
##########
superset-frontend/src/dashboard/constants.ts:
##########
@@ -43,6 +43,12 @@ export const FILTER_BAR_TABS_HEIGHT = 46;
export const BUILDER_SIDEPANEL_WIDTH = 374;
export const OVERWRITE_INSPECT_FIELDS = ['css', 'json_metadata.filter_scopes'];
export const EMPTY_CONTAINER_Z_INDEX = 10;
+// A grid component's hover menu floats outside the component's own box, so it
+// must paint above the drop targets it overlaps.
+export const HOVER_MENU_Z_INDEX = EMPTY_CONTAINER_Z_INDEX + 1;
+// A hover menu rendered above a column overlaps the bottom edge of whatever
+// sits above it, so resize handles have to win hit testing against it.
+export const RESIZE_HANDLE_Z_INDEX = HOVER_MENU_Z_INDEX + 1;
Review Comment:
Per the thread on `ResizableContainer.tsx`, this comment promises something
z-index can't deliver, since the chart and the column below it sit in separate
`translate3d` stacking contexts.
If the pointer-events change on the menu pans out, I'd drop
`RESIZE_HANDLE_Z_INDEX` (or reword this) so the constants don't document an
ordering that doesn't hold.
--
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]
