sadpandajoe commented on code in PR #45604:
URL: https://github.com/apache/superset/pull/45604#discussion_r4367115894
##########
superset-frontend/src/dashboard/components/ColorSchemeSelect.tsx:
##########
@@ -172,27 +173,42 @@ const ColorSchemeSelect = ({
return (
<>
- <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}
- onChange={onChange}
- 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}
+ onChange={onChange}
+ placeholder={t('Select scheme')}
+ value={currentScheme}
+ showSearch
+ getPopupContainer={triggerNode =>
Review Comment:
The modal-aware fallback here is the piece that keeps the palette scrolling
with its trigger inside `PropertiesModal`, but nothing exercises it: the
PropertiesModal and StylingSection tests mock `ColorSchemeSelect`, and the new
core tests cover `Select`'s own default rather than this override. If this
callback were replaced with `() => document.body`, every existing test would
still pass while the palette detached from the modal again. Could you add a
test that renders the real `ColorSchemeSelect` inside an
`.ant-modal-container`, opens it, and asserts the popup lands under that
container (plus one outside a modal asserting it lands under `document.body`)?
--
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]
