geido commented on code in PR #32514:
URL: https://github.com/apache/superset/pull/32514#discussion_r1998714684
##########
superset-frontend/src/components/Datasource/DatasourceEditor.test.jsx:
##########
@@ -197,7 +197,7 @@ describe('DatasourceEditor', () => {
});
describe('DatasourceEditor RTL', () => {
- jest.setTimeout(15000); // Extend timeout to 15s for this test
+ jest.setTimeout(20000); // Extend timeout to 20s for this test
Review Comment:
What made this necessary? Is there a performance degradation of some sort?
##########
superset-frontend/src/components/Select/styles.tsx:
##########
@@ -39,35 +38,41 @@ export const StyledContainer = styled.div<{ headerPosition:
string }>`
`}
`;
-export const StyledSelect = styled(AntdSelect, {
+export const StyledSelect = styled(Select, {
shouldForwardProp: prop => prop !== 'headerPosition' && prop !== 'oneLine',
})<{ headerPosition?: string; oneLine?: boolean }>`
${({ theme, headerPosition, oneLine }) => `
flex: ${headerPosition === 'left' ? 1 : 0};
- && .ant-select-selector {
- border-radius: ${theme.sizeUnit}px;
+ line-height: ${theme.sizeXL}px;
+
+ && .antd5-select-selection-search {
+ left: 0px;
+ }
+
+ && .antd5-select-selection-item, .antd5-select-selection-placeholder {
+ max-height: ${theme.sizeXL}px;
}
- // Open the dropdown when clicking on the suffix
- // This is fixed in version 4.16
- .ant-select-arrow .anticon:not(.ant-select-suffix) {
- pointer-events: none;
+ .antd5-select-selection-item::after {
+ height: 0;
+ display: block !important;
}
.select-all {
+ border-radius: 0;
border-bottom: 1px solid ${theme.colors.grayscale.light3};
Review Comment:
Can we re-check all usages of colors for the Select here and everywhere this
is implemented? Can we go back to vanilla?
##########
superset-frontend/src/explore/components/controls/SelectAsyncControl/index.tsx:
##########
@@ -96,10 +96,12 @@ const SelectAsyncControl = ({
endpoint: dataEndpoint,
})
.then(response => {
- const data = mutator
- ? mutator(response.json, value)
- : response.json.result;
- setOptions(data);
+ if (value) {
Review Comment:
Checking in again about the possibility of `null` values being valid
##########
superset-frontend/src/components/Select/utils.tsx:
##########
@@ -17,20 +17,40 @@
* under the License.
*/
import { ensureIsArray, t } from '@superset-ui/core';
-import AntdSelect, { LabeledValue as AntdLabeledValue } from 'antd/lib/select';
+import { Select } from 'antd-v5';
Review Comment:
Should `Select.Option` be exported from the `Select` component itself in our
wrapper?
##########
superset-frontend/src/components/Select/Select.tsx:
##########
@@ -264,8 +276,12 @@ const Select = forwardRef(
}
return [
SELECT_ALL_VALUE,
- ...selectAllEligible.map(opt => opt.value),
- ] as AntdLabeledValue[];
+ ...selectAllEligible
+ .map(opt => opt.value)
+ .filter(
+ (val): val is RawValue => val !== null && val !== undefined,
Review Comment:
Can we double check this? I see that here the expected value is `RawValue`
but I think I remember that in some cases `null` values could be acceptable in
the Select. @michael-s-molina you might have a better memory about this if you
can help?
See also following changes where this same logic is being applied.
##########
superset-frontend/src/components/Select/styles.tsx:
##########
@@ -18,8 +18,7 @@
*/
import { styled } from '@superset-ui/core';
import Icons from 'src/components/Icons';
-import { Spin, Tag } from 'antd';
-import AntdSelect from 'antd/lib/select';
+import { Spin, Tag, Select } from 'antd-v5';
Review Comment:
We can't be pulling directly from Ant Design. Can we refer to the existing
src/components/{component} (`Tag` has one, `Spin` would need a wrapper but
should be easy to include in this PR?)
##########
superset-frontend/src/features/databases/UploadDataModel/UploadDataModal.test.tsx:
##########
@@ -615,6 +615,8 @@ test('form without required fields', async () => {
});
test('CSV form post', async () => {
+ jest.setTimeout(10000);
Review Comment:
Why is this necessary?
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx:
##########
@@ -544,12 +544,20 @@ describe('AdhocFilterEditPopoverSimpleTabContent Advanced
data Type Test', () =>
);
expect(props.validHandler.lastCall.args[0]).toBe(true);
- const operatorValueField = screen.getByText('1 operator(s)');
+ const operatorValueField = screen.getByRole('combobox', {
+ name: 'Select operator',
+ });
+
+ userEvent.click(operatorValueField);
await act(async () => {
userEvent.type(operatorValueField, '{enter}');
});
- expect(screen.getByText('EQUALS')).toBeInTheDocument();
+ expect(
+ await screen.findByText('Equal to (=)', {
+ selector: '.ant-select-selection-item',
Review Comment:
Shouldn't this be using the `antd5` prefix?
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:
##########
@@ -67,19 +67,21 @@ export const EncryptedField = ({
<FormLabel>
{t('How do you want to enter service account credentials?')}
</FormLabel>
- <AntdSelect
+ <Select
defaultValue={uploadOption}
- style={{ width: '100%' }}
- onChange={option => setUploadOption(option)}
- >
- <AntdSelect.Option value={CredentialInfoOptions.JsonUpload}>
- {t('Upload JSON file')}
- </AntdSelect.Option>
-
- <AntdSelect.Option value={CredentialInfoOptions.CopyPaste}>
- {t('Copy and Paste JSON credentials')}
- </AntdSelect.Option>
- </AntdSelect>
+ css={{ width: '100%' }}
Review Comment:
Let's use the string annotation instead of object for consistency with the
rest of the codebase
##########
superset-frontend/src/components/Select/utils.tsx:
##########
@@ -104,12 +124,16 @@ export const sortSelectedFirstHelper = (
| RawValue[]
| AntdLabeledValue
| AntdLabeledValue[]
- | undefined,
-) =>
- selectValue && a.value !== undefined && b.value !== undefined
+ | undefined
+ | null,
Review Comment:
I think this was not here on purpose. Referring to my previous comment as
well.
--
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]
