geido commented on code in PR #43966:
URL: https://github.com/apache/superset/pull/43966#discussion_r3960102305
##########
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:
Minor: the docstring above says the row is reported as "bounded only by its
own content", but this branch falls back to `ROW_WIDTH` (250) rather than the
400px content width when there's no pixel clamp — and `ROW_WIDTH` is documented
a few lines up as the flex-bounded width, which is a different model.
It doesn't affect either assertion (test 1 never reaches the fallback while
the row holds 4 children, and test 2 ignores the `right` value), so this is
just the comment describing something the code doesn't do.
##########
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:
Non-blocking. Since this line has gone back and forth a few times
(`9ef93cea` -> `191c89cd`) and @msyavuz's `min(content, wrapper - button)`
question is still open, I went and measured it: I think `maxWidth` here is
inert, which dissolves the question rather than answering it.
The row already has `min-width: 0` and the initial `flex: 0 1 auto`, so
flex-shrink sizes it to roughly `wrapper - button` on its own. A `max-width`
set to the *full* wrapper width is always looser than that, and `max-width` is
a ceiling — it can't widen an element back up to it. So it never binds.
I checked in Chromium against a repro of this exact DOM (outer `display:
flex` wrapper, row with `min-width: 0`, bare `<button>` sibling) at four
settings: no fix at all; `overflow: hidden` only; `overflow: hidden` +
`max-width: wrapper`; `overflow: hidden` + `max-width: wrapper - button`. Row
and button geometry came out pixel-identical in all four, including with 20
items overflowing — the button was never pushed outside the wrapper, even with
no fix at all.
What did differ was the row's *children* spilling past the row's own
(already correctly shrunk) box. `overflow: hidden` alone clips that, so it
looks like `overflow: hidden` is the whole fix.
Caveat: I can't reproduce the original Edge frame, so if `maxWidth` turns
out to be empirically load-bearing there, keep it and ignore this. Otherwise
dropping it would simplify the change and close out the wrapper-vs-`wrapper -
button` thread for good.
--
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]
