sadpandajoe commented on code in PR #45604:
URL: https://github.com/apache/superset/pull/45604#discussion_r4367773821
##########
superset-frontend/plugins/plugin-chart-word-cloud/src/plugin/controls/ColorSchemeControl/index.tsx:
##########
@@ -304,28 +305,40 @@ const ColorSchemeControl = ({
/>
}
/>
- <Select
- css={css`
- width: 100%;
- & .ant-select-item.ant-select-item-group {
- padding-left: ${theme.sizeUnit}px;
- font-size: ${theme.fontSize}px;
- }
- & .ant-select-item-option-grouped {
- padding-left: ${theme.sizeUnit * 3}px;
- }
- `}
- aria-label={t('Select color scheme')}
- allowClear={clearable}
- disabled={hasDashboardScheme || hasSharedLabelsColor}
- onChange={handleOnChange}
- placeholder={t('Select scheme')}
- value={currentScheme}
- showSearch
- getPopupContainer={triggerNode => triggerNode.parentNode}
- options={options}
- optionFilterProps={['label', 'value', 'searchText']}
- />
+ <ClassNames>
+ {({ css: popupCss }) => (
+ <Select
+ css={css`
+ width: 100%;
+ `}
+ // the popup portals to document.body, out of the css prop's
+ // scope, so the grouped-option styles ride along on its root
+ classNames={{
+ popup: {
+ root: popupCss`
+ .ant-select-item.ant-select-item-group {
+ padding-left: ${theme.sizeUnit}px;
+ font-size: ${theme.fontSize}px;
+ }
+ .ant-select-item-option-grouped {
+ padding-left: ${theme.sizeUnit * 3}px;
+ }
+ `,
+ },
+ }}
+ aria-label={t('Select color scheme')}
+ allowClear={clearable}
+ disabled={hasDashboardScheme || hasSharedLabelsColor}
+ onChange={handleOnChange}
+ placeholder={t('Select scheme')}
+ value={currentScheme}
+ showSearch
+ getPopupContainer={() => document.body}
Review Comment:
This plugin carries its own copy of the portaled-popup change, but
`test/ColorSchemeControl.test.tsx` only covers rendering and selection.
Reverting this `getPopupContainer` to `triggerNode.parentNode`, or moving the
grouped-option styles back under the trigger's `css`, would leave every test
green (the new Explore control test exercises a different component). Could you
add a test here with registered grouped palettes that asserts the opened popup
is outside the control's container and that the grouped-option padding rule is
attached to the popup root?
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/GroupByFilterCard.test.tsx:
##########
@@ -532,3 +532,28 @@ test('shows no message while at least one allowed column
remains', async () => {
expect(await screen.findByText('Country')).toBeInTheDocument();
expect(screen.queryByText(allowlistGoneMessage)).not.toBeInTheDocument();
});
+
+test('horizontal layout keeps the 200-400px popup width constraints', async ()
=> {
+ // The popup portals to document.body, outside HorizontalFormItem's styled
+ // scope, so the width clamp must arrive as inline popup styles instead.
+ fetchMock.get('glob:*/api/v1/dataset/325', {
+ result: { table_name: 'Vehicle Sales', columns: [{ column_name: 'city' }]
},
+ });
+
+ render(
+ <GroupByFilterCard
+ customizationItem={customization([{ datasetId: 325 }])}
+ orientation="horizontal"
+ />,
+ { useRedux: true, initialState },
+ );
+
+ await userEvent.click(await screen.findByRole('combobox'));
+ await waitFor(() => {
+ expect(document.querySelector('.ant-select-dropdown')).not.toBeNull();
+ });
+ expect(document.querySelector('.ant-select-dropdown')).toHaveStyle({
+ minWidth: '200px',
Review Comment:
This only asserts the popup's min/max width, so putting
`getPopupContainer={trigger => trigger.parentNode}` back on this caller would
keep the widths (they ride on the inline styles) and still pass while the
double scrollbar returned in the horizontal filter bar. The shared Select tests
can't catch it either, because this caller passes its own container. Could you
render the card inside an `overflow: auto` wrapper and assert the opened
`.ant-select-dropdown` is not contained in it, as `ColorSchemeSelect.test.tsx`
does?
--
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]
