rusackas commented on code in PR #42600:
URL: https://github.com/apache/superset/pull/42600#discussion_r3685472720
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.stories.tsx:
##########
@@ -338,6 +338,46 @@ AtEveryCorner.parameters = {
},
};
+const manyPermissionLabels = [
+ 'can read SavedQuery',
+ 'can write SavedQuery',
+ 'can read Database',
+ 'can read Query',
+ 'can activate TabStateView',
+ 'can copy clipboard Superset',
+ 'can csv Superset',
+ 'can delete query TabStateView',
+ 'can delete TableSchemaView',
+ 'can delete TabStateView',
+ 'can estimate query cost SQLLab',
+ 'can execute sql query SQLLab',
+];
+const manyPermissionOptions = Array.from({ length: 32 }, (_, i) => ({
+ label:
+ manyPermissionLabels[i % manyPermissionLabels.length] +
+ (i >= manyPermissionLabels.length ? ` ${i}` : ''),
+ value: `perm_${i}`,
+}));
+
+/**
+ * Regression coverage for a narrow container where selected tags wrap to
+ * multiple rows (e.g. the Role edit modal's Permissions field, which has no
+ * `maxTagCount` override and defaults to wrapping at 4 tags per row). See
+ * https://github.com/apache/superset/issues/39339.
+ */
+export const ManySelectedValuesWrapping = () => (
+ <div style={{ width: 600 }}>
+ <Select
+ ariaLabel="many-selected-values-select"
+ mode="multiple"
+ showSearch
+ options={manyPermissionOptions}
+ value={manyPermissionOptions.map(o => o.value)}
+ getPopupContainer={trigger => trigger.parentElement as HTMLElement}
Review Comment:
I don't think this holds up. The story never claims to render all 32 tags on
screen — it deliberately omits `maxTagCount` so it defaults to 4, mirroring the
real-world case (Role edit modal's Permissions field, issue #39339). Even with
just 4 tags + the "+28 ..." overflow tag in a 600px container, the combined
width already exceeds 600px and wraps to multiple rows, which is exactly the
regression this story is meant to catch. The 32-option list is just realistic
permission-style data, not a claim that all of it renders.
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.test.tsx:
##########
@@ -898,6 +898,28 @@ test('Renders only an overflow tag if dropdown is open in
oneLine mode', async (
expect(withinSelector.getByText('+ 2 ...')).toBeVisible();
});
+// Regression test for the bug described in:
https://github.com/apache/superset/issues/39339
+// The AntD v6 upgrade renamed `.ant-select-selection-overflow` (the tag
+// container) to `.ant-select-content`, and a CSS rule meant only to cap the
+// height of individual tags/placeholder text was mistakenly widened to also
+// match the renamed container class. That capped the whole multi-row tag
+// container to a single line's height, so wrapped rows spilled outside the
+// select's border, the dropdown arrow mis-centered against the wrong height,
+// and the search input (rendered last in the wrapped row) became invisible
+// once the dropdown opened.
+test('does not cap the tag container to a single line when tags wrap to
multiple rows', () => {
+ render(
+ <Select
+ {...defaultProps}
+ value={OPTIONS.slice(0, 8)}
+ mode="multiple"
+ maxTagCount={6}
+ />,
+ );
+ const content = getElementByClassName('.ant-select-content');
+ expect(content).not.toHaveStyle({ maxHeight: '32px' });
Review Comment:
I think this misreads what the test is checking. The bug was that `&&
.ant-select-content` in styles.tsx unconditionally applied `max-height:
theme.sizeXL px` to the tag container, regardless of tag count or select width
— it's not gated on wrapping actually occurring. The test just verifies that
class-based rule is gone from the container after the fix; it doesn't need real
flex-wrap layout (which jsdom doesn't simulate anyway) to catch a regression in
that CSS selector. maxTagCount={6} with 8 values is enough to render the
container with an overflow tag and check its style, which is all this needs to
verify.
--
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]
