EnxDev commented on code in PR #43966:
URL: https://github.com/apache/superset/pull/43966#discussion_r3965641485
##########
superset-frontend/packages/superset-ui-core/src/components/DropdownContainer/DropdownContainer.tsx:
##########
@@ -330,7 +330,18 @@ export const DropdownContainer = forwardRef(
min-width: 0px;
`}
data-test="container"
- style={style}
+ style={
+ recalculating
+ ? {
+ ...style,
+ /* Clamp the transient all-items row to the wrapper width.
+ * `width` is 0 until the first resize callback, so fall back
+ * to the consumer's value rather than clamping to nothing.
*/
+ maxWidth: width || style?.maxWidth,
Review Comment:
Dropped it in fb93890. Your reading holds and the CSS follows: with
`min-width: 0` and `flex: 0 1 auto`, flex-shrink already sizes the row to
roughly `wrapper - button`, and a `max-width` at the *full* wrapper width is
looser than that used width, so it never binds. It also couldn't have helped if
it had bound — a row allowed to reach the full wrapper width pushes the sibling
button out entirely, which is the same point bito raised in the first round. So
the clamp was either inert or insufficient, and `overflow: hidden` clipping the
spilling children is the actual fix. That also dissolves @msyavuz's `wrapper -
button` question rather than leaving it open.
The zero-width guard and its test went with it, since there is no longer a
width to guard.
On your caveat: I can't reproduce the Edge frame from here either, and the
"after" recording in the PR description was made with `maxWidth` present, so
`overflow: hidden` alone has not been observed against the real frame. I'm
re-running the Edge repro before this merges; if the clamp turns out to be
empirically load-bearing there, restoring it is a one-line revert of the
component change and I'll say so here.
##########
superset-frontend/packages/superset-ui-core/src/components/DropdownContainer/DropdownContainer.test.tsx:
##########
@@ -178,3 +180,98 @@ test('component renders and functions without throwing
errors', () => {
// Basic functionality test
expect(screen.getByText('Element 1')).toBeInTheDocument();
});
+
+const WRAPPER_WIDTH = 300;
+const ITEM_WIDTH = 100;
+/* Width the flex layout leaves the row once the trigger button is laid out. */
+const ROW_WIDTH = 250;
+
+/**
+ * Lays items out at ITEM_WIDTH each and reports the item row as bounded only
+ * by its own content, which is the frame Edge can paint before the flex layout
+ * bounds the row. An inline `max-width` in pixels is the only bound left, so
+ * the mock honors it and `onRowMeasure` receives the row, and the right edge
+ * the overflow calculation sees, while the row holds every item.
+ */
+const mockBoundingRects = (
+ onRowMeasure: (row: HTMLElement, right: number) => void,
+) => {
+ const getBoundingClientRect: (this: HTMLElement) => DOMRect = function () {
+ let right: number;
+ if (this.dataset.test === 'container') {
+ const clamp = /^(\d+(?:\.\d+)?)px$/.exec(this.style.maxWidth);
+ right = Math.min(
+ this.children.length * ITEM_WIDTH,
+ clamp ? Number(clamp[1]) : ROW_WIDTH,
Review Comment:
Fixed in fb93890 — and this turned out to be more than a comment inaccuracy,
so thanks for pulling on it.
Removing the pixel-clamp branch from the mock made it report the row at its
content width unconditionally, and the three-item row then stopped overflowing
at all: the trigger never rendered and the
`getByTestId('dropdown-container-btn')` precondition failed. The `ROW_WIDTH`
fallback you flagged as "a different model" was quietly the thing making the
steady-state row overflow.
So the mock now states both regimes instead of one: `ROW_WIDTH`
(flex-bounded) in the steady state, the row's own content width only in the
all-items frame, which is the frame Edge can paint. The docstring describes
that, and the two clamp tests collapse into one that records
`row.style.overflow` at every all-items measurement and asserts the first is
`hidden` — beating the consumer's `overflow: visible` — and the last is
`visible`.
--
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]
