You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/03/18 17:53:10 UTC
[PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
msyavuz opened a new pull request, #32729:
URL: https://github.com/apache/superset/pull/32729
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Migrate Form component to Ant Design 5.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
WIP
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on PR #32729:
URL: https://github.com/apache/superset/pull/32729#issuecomment-2768339209
> Curious if you've tried switching to dark mode to make sure form components adapt nicely while in V5 (?)
Yes, i did! It doesn't have any problems in dark mode as far as i can tell. Though some parts look off because both `Collapse` and `Select` are not migrated yet.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008216197
##########
superset-frontend/src/components/Form/FormItem.tsx:
##########
@@ -16,24 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import Form from 'antd/lib/form'; // TODO: Remove antd
+import { Form } from 'antd-v5';
import { styled } from '@superset-ui/core';
const StyledItem = styled(Form.Item)`
${({ theme }) => `
- .ant-form-item-label {
- padding-bottom: ${theme.sizeUnit}px;
+ .antd5-form-item {
+ margin-bottom: 0;
+ }
+ .antd5-form-item-label {
& > label {
- font-size: ${theme.fontSizeSM}px;
- color: ${theme.colors.grayscale.base};
-
- &.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &.antd5-form-item-required:not(.antd5-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
+ visibility: visible;
color: ${theme.colorError};
font-size: ${theme.fontSizeSM}px;
Review Comment:
Same, this is a custom after element so fontSize is necessary
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021866233
##########
superset-frontend/src/filters/components/TimeColumn/TimeColumnFilterPlugin.tsx:
##########
@@ -25,9 +25,8 @@ import {
} from '@superset-ui/core';
import { useEffect, useState } from 'react';
import { Select } from 'src/components';
Review Comment:
No changes here either
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008220646
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -29,7 +29,7 @@ import {
useTheme,
} from '@superset-ui/core';
import { useDispatch } from 'react-redux';
-import { AntdForm } from 'src/components';
+import Form from 'src/components/Form/Form';
Review Comment:
I think something might be messed up in the branch because i see this as `import { Form } from 'src/components/Form';` on local 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008215208
##########
superset-frontend/src/components/Form/FormItem.tsx:
##########
@@ -16,24 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import Form from 'antd/lib/form'; // TODO: Remove antd
+import { Form } from 'antd-v5';
import { styled } from '@superset-ui/core';
const StyledItem = styled(Form.Item)`
${({ theme }) => `
- .ant-form-item-label {
- padding-bottom: ${theme.sizeUnit}px;
+ .antd5-form-item {
+ margin-bottom: 0;
+ }
+ .antd5-form-item-label {
& > label {
- font-size: ${theme.fontSizeSM}px;
- color: ${theme.colors.grayscale.base};
-
- &.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &.antd5-form-item-required:not(.antd5-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
+ visibility: visible;
color: ${theme.colorError};
Review Comment:
We don't use the Ant Design's `*`. This is our own `*` which needs color. Ant design puts `*` before the label by default and we want it after
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021864048
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -19,10 +19,10 @@
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { omit } from 'lodash';
import { Input } from 'src/components/Input';
-import { FormItem } from 'src/components/Form';
+import { Form, FormItem } from 'src/components/Form';
import jsonStringify from 'json-stringify-pretty-compact';
import Button from 'src/components/Button';
-import { AntdForm, AsyncSelect, Col, Row } from 'src/components';
+import { AsyncSelect, Col, Row } from 'src/components';
Review Comment:
and here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008215208
##########
superset-frontend/src/components/Form/FormItem.tsx:
##########
@@ -16,24 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import Form from 'antd/lib/form'; // TODO: Remove antd
+import { Form } from 'antd-v5';
import { styled } from '@superset-ui/core';
const StyledItem = styled(Form.Item)`
${({ theme }) => `
- .ant-form-item-label {
- padding-bottom: ${theme.sizeUnit}px;
+ .antd5-form-item {
+ margin-bottom: 0;
+ }
+ .antd5-form-item-label {
& > label {
- font-size: ${theme.fontSizeSM}px;
- color: ${theme.colors.grayscale.base};
-
- &.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &.antd5-form-item-required:not(.antd5-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
+ visibility: visible;
color: ${theme.colorError};
Review Comment:
We don't use the Ant Design's `*`. This is our own `*` which needs color. Ant design puts `*` before the label by default
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021833207
##########
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx:
##########
@@ -18,7 +18,7 @@
*/
import { useState } from 'react';
import { t, styled } from '@superset-ui/core';
-import { AntdForm, Col, Row } from 'src/components';
+import { Col, Row } from 'src/components';
Review Comment:
follow this https://github.com/apache/superset/pull/32729/files#r2008119444
##########
superset-frontend/src/dashboard/components/PropertiesModal/index.tsx:
##########
@@ -19,10 +19,10 @@
import { useCallback, useEffect, useMemo, useRef, useState } from 'react';
import { omit } from 'lodash';
import { Input } from 'src/components/Input';
-import { FormItem } from 'src/components/Form';
+import { Form, FormItem } from 'src/components/Form';
import jsonStringify from 'json-stringify-pretty-compact';
import Button from 'src/components/Button';
-import { AntdForm, AsyncSelect, Col, Row } from 'src/components';
+import { AsyncSelect, Col, Row } from 'src/components';
Review Comment:
follow this https://github.com/apache/superset/pull/32729/files#r2008119444
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:
##########
@@ -19,7 +19,7 @@
import { useRef, useState } from 'react';
import { SupersetTheme, t } from '@superset-ui/core';
import { Button, AntdSelect } from 'src/components';
Review Comment:
follow this https://github.com/apache/superset/pull/32729/files#r2008119444
##########
superset-frontend/src/filters/components/TimeColumn/TimeColumnFilterPlugin.tsx:
##########
@@ -25,9 +25,8 @@ import {
} from '@superset-ui/core';
import { useEffect, useState } from 'react';
import { Select } from 'src/components';
Review Comment:
follow this https://github.com/apache/superset/pull/32729/files#r2008119444
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -103,6 +103,9 @@ const FilterBarEmptyStateContainer = styled.div`
`;
const FilterControlsWrapper = styled.div`
+ display: flex;
+ flex-direction: column;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
padding: ${({ theme }) => theme.sizeUnit * 4}px;
Review Comment:
We should use tagged template literals?
##########
superset-frontend/src/features/databases/UploadDataModel/index.tsx:
##########
@@ -35,20 +35,14 @@ import Modal from 'src/components/Modal';
import Button from 'src/components/Button';
import { Switch, SwitchProps } from 'src/components/Switch';
import Collapse from 'src/components/Collapse';
-import {
- AntdForm,
- AsyncSelect,
- Col,
- Row,
- Select,
- Upload,
-} from 'src/components';
+import { AsyncSelect, Col, Row, Select, Upload } from 'src/components';
Review Comment:
follow this https://github.com/apache/superset/pull/32729/files#r2008119444
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021864902
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:
##########
@@ -19,7 +19,7 @@
import { useRef, useState } from 'react';
import { SupersetTheme, t } from '@superset-ui/core';
import { Button, AntdSelect } from 'src/components';
Review Comment:
There are no changes here so no merge conflicts
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021854786
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -103,6 +103,9 @@ const FilterBarEmptyStateContainer = styled.div`
`;
const FilterControlsWrapper = styled.div`
+ display: flex;
+ flex-direction: column;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
padding: ${({ theme }) => theme.sizeUnit * 4}px;
Review Comment:
I am not sure i understand. This is a tagged template literal already.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021863165
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -103,6 +103,9 @@ const FilterBarEmptyStateContainer = styled.div`
`;
const FilterControlsWrapper = styled.div`
+ display: flex;
+ flex-direction: column;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
padding: ${({ theme }) => theme.sizeUnit * 4}px;
Review Comment:
`${({ theme,}) => padding: ${theme.gridUnit * 2}px; ` to avoid the repetition for `${({ theme,}) =>`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2005107003
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx:
##########
@@ -25,16 +25,17 @@ import {
useChangeEffect,
getClientErrorObject,
} from '@superset-ui/core';
-import { Select, FormInstance } from 'src/components';
+import { Select } from 'src/components';
+import { FormInstance } from 'src/components/Form/Form';
import { useToasts } from 'src/components/MessageToasts/withToasts';
Review Comment:
Maybe it would be better if we would handle everything in the `index.tsx` of `src/components/Form` so that we won't have to import like `Form/Form` . I've seen that's the common pattern. What do you think?
##########
superset-frontend/src/components/Form/Form.tsx:
##########
@@ -16,21 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import AntdForm, { FormProps } from 'antd/lib/form'; // TODO: Remove antd
-import { styled } from '@superset-ui/core';
+import { Form as AntdForm } from 'antd-v5';
+import { FormProps, FormInstance, FormItemProps } from 'antd-v5/es/form';
-const StyledForm = styled(AntdForm)`
- &.ant-form label {
- font-size: ${({ theme }) => theme.fontSizeSM}px;
- }
- .ant-form-item {
- margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
- }
-`;
-
-export default function Form(props: FormProps) {
- return <StyledForm {...props} />;
+function Form(props: FormProps) {
+ return <AntdForm {...props} />;
}
-export type { FormProps };
+export default Object.assign(Form, {
+ useForm: AntdForm.useForm,
+ Item: AntdForm.Item,
+ List: AntdForm.List,
+ ErrorList: AntdForm.ErrorList,
+ Provider: AntdForm.Provider,
+});
+
Review Comment:
I think this can be reduced to this => `const Form = Object.assign(
(props: FormProps) => <AntdForm {...props} />,
AntdForm,
) as typeof AntdForm;`
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx:
##########
@@ -42,7 +42,7 @@ const Wrapper = styled.div`
padding: 0px ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const CleanFormItem = styled(AntdForm.Item)`
+const CleanFormItem = styled(Form.Item)`
margin-bottom: 0;
Review Comment:
I haven't checked but if the only the `FormItem is used here, then maybe we can import the FormItem directly instead of the whole form component
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #32729:
URL: https://github.com/apache/superset/pull/32729#issuecomment-2737463270
Marking this as a Draft since it's work in progress :) Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021865865
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -103,6 +103,9 @@ const FilterBarEmptyStateContainer = styled.div`
`;
const FilterControlsWrapper = styled.div`
+ display: flex;
+ flex-direction: column;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
padding: ${({ theme }) => theme.sizeUnit * 4}px;
Review Comment:
`${({ theme,}) => padding: ${theme.sizeUnit * 2}px; ` to avoid the repetition for `${({ theme,}) =>`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008206918
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx:
##########
@@ -141,13 +142,12 @@ const controlsOrder: ControlKey[] = [
export const StyledFormItem = styled(FormItem)<{ expanded: boolean }>`
width: ${({ expanded }) => (expanded ? '49%' : `${FORM_ITEM_WIDTH}px`)};
- margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
- & .ant-form-item-label {
+ & .antd5-form-item-label {
Review Comment:
Before removing paddings:
After removing paddings:
I think it might be a design choice at the time?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008217031
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx:
##########
@@ -42,7 +42,7 @@ const Wrapper = styled.div`
padding: 0px ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const CleanFormItem = styled(AntdForm.Item)`
+const CleanFormItem = styled(Form.Item)`
margin-bottom: 0;
Review Comment:
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021868577
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:
##########
@@ -19,7 +19,7 @@
import { useRef, useState } from 'react';
import { SupersetTheme, t } from '@superset-ui/core';
import { Button, AntdSelect } from 'src/components';
Review Comment:
Why?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021868577
##########
superset-frontend/src/features/databases/DatabaseModal/DatabaseConnectionForm/EncryptedField.tsx:
##########
@@ -19,7 +19,7 @@
import { useRef, useState } from 'react';
import { SupersetTheme, t } from '@superset-ui/core';
import { Button, AntdSelect } from 'src/components';
Review Comment:
Why?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2005243617
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx:
##########
@@ -42,7 +42,7 @@ const Wrapper = styled.div`
padding: 0px ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const CleanFormItem = styled(AntdForm.Item)`
+const CleanFormItem = styled(Form.Item)`
margin-bottom: 0;
Review Comment:
Done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008213279
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx:
##########
@@ -42,7 +42,7 @@ const Wrapper = styled.div`
padding: 0px ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const CleanFormItem = styled(AntdForm.Item)`
+const CleanFormItem = styled(Form.Item)`
margin-bottom: 0;
Review Comment:
This is weird, i see the current code as `FormItem`. But in here it seems different
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021863165
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/Vertical.tsx:
##########
@@ -103,6 +103,9 @@ const FilterBarEmptyStateContainer = styled.div`
`;
const FilterControlsWrapper = styled.div`
+ display: flex;
+ flex-direction: column;
+ gap: ${({ theme }) => theme.sizeUnit * 2}px;
padding: ${({ theme }) => theme.sizeUnit * 4}px;
Review Comment:
`${({ theme,}) => padding: ${theme.gridUnit * 2}px; ` to avoid the repetition for `${({ theme,}) =>`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #32729:
URL: https://github.com/apache/superset/pull/32729#issuecomment-2767638662
Curious if you've tried switching to dark mode to make sure form components adapt nicely while in V5 (?)
If you rebase on top of the latest `template_less` branch, should be as easy as opening the ThemeEditor (top right navbar modal trigger) and setting the theme in the dropdown to `supersetDark` ->
<img width="1570" alt="Screenshot 2025-03-31 at 4 22 52 PM" src="https://github.com/user-attachments/assets/89ae17b8-11c6-4825-9fd3-07878b9451ce" />
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2002695197
##########
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx:
##########
@@ -78,30 +78,30 @@ const HorizontalOverflowFilterControlTitleBox = styled(
const VerticalFilterControlContainer = styled(Form)`
width: 100%;
- && .ant-form-item-label > label {
+ && .antd5-form-item-label > label {
text-transform: none;
width: 100%;
padding-right: ${({ theme }) => theme.sizeUnit * 11}px;
}
- .ant-form-item-tooltip {
+ .antd5-form-item-tooltip {
margin-bottom: ${({ theme }) => theme.sizeUnit}px;
}
`;
const HorizontalFilterControlContainer = styled(Form)`
- && .ant-form-item-label > label {
+ && .antd5-form-item-label > label {
margin-bottom: 0;
text-transform: none;
}
- .ant-form-item-tooltip {
+ .antd5-form-item-tooltip {
margin-bottom: ${({ theme }) => theme.sizeUnit}px;
}
`;
Review Comment:
### Duplicated styled component styles <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The styled components for Vertical and Horizontal containers have duplicated CSS rules and similar structures, violating the DRY principle.
###### Why this matters
Code duplication increases maintenance overhead and the risk of inconsistencies when changes are needed.
###### Suggested change ∙ *Feature Preview*
Create a base styled component with common styles and extend it for specific variations:
```typescript
const BaseFilterControlContainer = styled(Form)`
.antd5-form-item-tooltip {
margin-bottom: ${({ theme }) => theme.sizeUnit}px;
}
&& .antd5-form-item-label > label {
text-transform: none;
}
`;
const VerticalFilterControlContainer = styled(BaseFilterControlContainer)`
width: 100%;
&& .antd5-form-item-label > label {
width: 100%;
padding-right: ${({ theme }) => theme.sizeUnit * 11}px;
}
`;
const HorizontalFilterControlContainer = styled(BaseFilterControlContainer)`
&& .antd5-form-item-label > label {
margin-bottom: 0;
}
`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ae875a6-a203-4e15-ae9b-3598d590b5eb/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ae875a6-a203-4e15-ae9b-3598d590b5eb?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ae875a6-a203-4e15-ae9b-3598d590b5eb?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ae875a6-a203-4e15-ae9b-3598d590b5eb?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5ae875a6-a203-4e15-ae9b-3598d590b5eb)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:377f8022-fe0b-4a68-85b4-0fefc35c8cef -->
[](377f8022-fe0b-4a68-85b4-0fefc35c8cef)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2019103850
##########
superset-frontend/src/components/Flex/Flex.stories.tsx:
##########
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Flex, FlexProps } from 'src/components/Flex';
+import { css } from '@superset-ui/core';
+
+export default {
+ title: 'Flex',
+ component: Flex,
+};
+
+export const InteractiveFlex = (args: FlexProps) => (
+ <Flex
+ {...args}
+ css={css`
+ width: 100%;
+ height: 90vh;
+ `}
+ >
+ {new Array(20).fill(null).map((_, i) => (
+ <p key={i}>Item</p>
+ ))}
+ </Flex>
+);
+
+InteractiveFlex.args = {
+ vertical: false,
+ wrap: 'nowrap',
+ justify: 'normal',
+ align: 'normal',
+ flex: 'normal',
+ gap: 'small',
+};
+
+InteractiveFlex.argTypes = {
+ vertical: {
+ control: { type: 'boolean' },
+ type: { name: 'boolean', required: false },
+ },
+ wrap: {
+ control: { type: 'select' },
+ options: ['nowrap', 'wrap', 'wrap-reverse', false, true],
Review Comment:
### Type Inconsistency in Wrap Property Options <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The wrap property options include boolean values (false, true) which are inconsistent with the string type declaration and may cause type conflicts.
###### Why this matters
This type mismatch could lead to runtime errors when the component receives boolean values, as the type system expects string values. This contradicts the TypeScript type safety principles.
###### Suggested change ∙ *Feature Preview*
Remove the boolean options and keep only string values that match the type declaration:
```typescript
options: ['nowrap', 'wrap', 'wrap-reverse'],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7d1ac8ea-4c3a-4317-87f8-ab1a09d837dc/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7d1ac8ea-4c3a-4317-87f8-ab1a09d837dc?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7d1ac8ea-4c3a-4317-87f8-ab1a09d837dc?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7d1ac8ea-4c3a-4317-87f8-ab1a09d837dc?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/7d1ac8ea-4c3a-4317-87f8-ab1a09d837dc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:fd3b36e5-aef5-47f1-9d7f-5f04c9858544 -->
[](fd3b36e5-aef5-47f1-9d7f-5f04c9858544)
##########
superset-frontend/src/components/Flex/Flex.stories.tsx:
##########
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { Flex, FlexProps } from 'src/components/Flex';
+import { css } from '@superset-ui/core';
+
+export default {
+ title: 'Flex',
+ component: Flex,
+};
+
+export const InteractiveFlex = (args: FlexProps) => (
+ <Flex
+ {...args}
+ css={css`
+ width: 100%;
+ height: 90vh;
+ `}
+ >
+ {new Array(20).fill(null).map((_, i) => (
+ <p key={i}>Item</p>
+ ))}
Review Comment:
### Non-configurable demo items in story <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The story component uses hardcoded array creation with map for demonstration items rather than making the items configurable through story args.
###### Why this matters
This reduces the reusability and flexibility of the story component for testing different scenarios. It violates the Open-Closed Principle by not allowing extension without modification.
###### Suggested change ∙ *Feature Preview*
Make items configurable through story args:
```typescript
export const InteractiveFlex = ({ items = 20, ...args }: FlexProps & { items?: number }) => (
<Flex {...args}>
{Array.from({ length: items }, (_, i) => (
<p key={i}>Item {i + 1}</p>
))}
</Flex>
);
InteractiveFlex.args = {
items: 20,
// ... other args
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc8c18fb-344b-445e-9d44-6263a1f9fb35/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc8c18fb-344b-445e-9d44-6263a1f9fb35?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc8c18fb-344b-445e-9d44-6263a1f9fb35?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc8c18fb-344b-445e-9d44-6263a1f9fb35?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/bc8c18fb-344b-445e-9d44-6263a1f9fb35)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:c0570da8-064d-49f4-9834-8f314f7a40c1 -->
[](c0570da8-064d-49f4-9834-8f314f7a40c1)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2008109333
##########
superset-frontend/src/components/Form/FormItem.tsx:
##########
@@ -16,24 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import Form from 'antd/lib/form'; // TODO: Remove antd
+import { Form } from 'antd-v5';
import { styled } from '@superset-ui/core';
const StyledItem = styled(Form.Item)`
${({ theme }) => `
- .ant-form-item-label {
- padding-bottom: ${theme.sizeUnit}px;
+ .antd5-form-item {
+ margin-bottom: 0;
+ }
+ .antd5-form-item-label {
& > label {
- font-size: ${theme.fontSizeSM}px;
- color: ${theme.colors.grayscale.base};
-
- &.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &.antd5-form-item-required:not(.antd5-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
+ visibility: visible;
color: ${theme.colorError};
Review Comment:
Isn't the color here already the correct one in vanilla?
##########
superset-frontend/src/components/Form/FormItem.tsx:
##########
@@ -16,24 +16,23 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import Form from 'antd/lib/form'; // TODO: Remove antd
+import { Form } from 'antd-v5';
import { styled } from '@superset-ui/core';
const StyledItem = styled(Form.Item)`
${({ theme }) => `
- .ant-form-item-label {
- padding-bottom: ${theme.sizeUnit}px;
+ .antd5-form-item {
+ margin-bottom: 0;
+ }
+ .antd5-form-item-label {
& > label {
- font-size: ${theme.fontSizeSM}px;
- color: ${theme.colors.grayscale.base};
-
- &.ant-form-item-required:not(.ant-form-item-required-mark-optional) {
+ &.antd5-form-item-required:not(.antd5-form-item-required-mark-optional) {
&::before {
display: none;
}
&::after {
display: inline-block;
+ visibility: visible;
color: ${theme.colorError};
font-size: ${theme.fontSizeSM}px;
Review Comment:
Same here, do we need to change the font or vanilla is good?
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FilterScope/FilterScope.tsx:
##########
@@ -42,7 +42,7 @@ const Wrapper = styled.div`
padding: 0px ${({ theme }) => theme.sizeUnit * 4}px;
`;
-const CleanFormItem = styled(AntdForm.Item)`
+const CleanFormItem = styled(Form.Item)`
margin-bottom: 0;
Review Comment:
It seems we are still importing `Form` here?
##########
superset-frontend/src/explore/components/PropertiesModal/index.tsx:
##########
@@ -21,7 +21,7 @@ import { ChangeEvent, useMemo, useState, useCallback, useEffect } from 'react';
import Modal from 'src/components/Modal';
import { Input, TextArea } from 'src/components/Input';
import Button from 'src/components/Button';
-import { AsyncSelect, Row, Col, AntdForm } from 'src/components';
+import { AsyncSelect, Row, Col } from 'src/components';
Review Comment:
@EnxDev is working on a PR that will change these imports. Could be better to wait for that to be merged or at least keep an eye on these.
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx:
##########
@@ -29,7 +29,7 @@ import {
useTheme,
} from '@superset-ui/core';
import { useDispatch } from 'react-redux';
-import { AntdForm } from 'src/components';
+import Form from 'src/components/Form/Form';
Review Comment:
Should not this be `import { Form } from 'src/components/Form';`
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch merged PR #32729:
URL: https://github.com/apache/superset/pull/32729
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2021863226
##########
superset-frontend/src/features/databases/DatabaseModal/SSHTunnelForm.tsx:
##########
@@ -18,7 +18,7 @@
*/
import { useState } from 'react';
import { t, styled } from '@superset-ui/core';
-import { AntdForm, Col, Row } from 'src/components';
+import { Col, Row } from 'src/components';
Review Comment:
Thanks for marking these, i already have my eye on these imports and will fix merge conflicts when your pr is merged
##########
superset-frontend/src/features/databases/UploadDataModel/index.tsx:
##########
@@ -35,20 +35,14 @@ import Modal from 'src/components/Modal';
import Button from 'src/components/Button';
import { Switch, SwitchProps } from 'src/components/Switch';
import Collapse from 'src/components/Collapse';
-import {
- AntdForm,
- AsyncSelect,
- Col,
- Row,
- Select,
- Upload,
-} from 'src/components';
+import { AsyncSelect, Col, Row, Select, Upload } from 'src/components';
Review Comment:
same here
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2005252424
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.tsx:
##########
@@ -25,16 +25,17 @@ import {
useChangeEffect,
getClientErrorObject,
} from '@superset-ui/core';
-import { Select, FormInstance } from 'src/components';
+import { Select } from 'src/components';
+import { FormInstance } from 'src/components/Form/Form';
import { useToasts } from 'src/components/MessageToasts/withToasts';
Review Comment:
Makes sense, done!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #32729:
URL: https://github.com/apache/superset/pull/32729#discussion_r2005208062
##########
superset-frontend/src/components/Form/Form.tsx:
##########
@@ -16,21 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import AntdForm, { FormProps } from 'antd/lib/form'; // TODO: Remove antd
-import { styled } from '@superset-ui/core';
+import { Form as AntdForm } from 'antd-v5';
+import { FormProps, FormInstance, FormItemProps } from 'antd-v5/es/form';
-const StyledForm = styled(AntdForm)`
- &.ant-form label {
- font-size: ${({ theme }) => theme.fontSizeSM}px;
- }
- .ant-form-item {
- margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
- }
-`;
-
-export default function Form(props: FormProps) {
- return <StyledForm {...props} />;
+function Form(props: FormProps) {
+ return <AntdForm {...props} />;
}
-export type { FormProps };
+export default Object.assign(Form, {
+ useForm: AntdForm.useForm,
+ Item: AntdForm.Item,
+ List: AntdForm.List,
+ ErrorList: AntdForm.ErrorList,
+ Provider: AntdForm.Provider,
+});
+
Review Comment:
There are some type issues in usages with using that syntax. I think being a bit more explicit might be okay here? What do you think
##########
superset-frontend/src/components/Form/Form.tsx:
##########
@@ -16,21 +16,19 @@
* specific language governing permissions and limitations
* under the License.
*/
-// eslint-disable-next-line no-restricted-imports
-import AntdForm, { FormProps } from 'antd/lib/form'; // TODO: Remove antd
-import { styled } from '@superset-ui/core';
+import { Form as AntdForm } from 'antd-v5';
+import { FormProps, FormInstance, FormItemProps } from 'antd-v5/es/form';
-const StyledForm = styled(AntdForm)`
- &.ant-form label {
- font-size: ${({ theme }) => theme.fontSizeSM}px;
- }
- .ant-form-item {
- margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
- }
-`;
-
-export default function Form(props: FormProps) {
- return <StyledForm {...props} />;
+function Form(props: FormProps) {
+ return <AntdForm {...props} />;
}
-export type { FormProps };
+export default Object.assign(Form, {
+ useForm: AntdForm.useForm,
+ Item: AntdForm.Item,
+ List: AntdForm.List,
+ ErrorList: AntdForm.ErrorList,
+ Provider: AntdForm.Provider,
+});
+
Review Comment:
There are some type issues in usages with using that syntax. I think being a bit more explicit might be okay here? What do you think?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] refactor(form): Migrate Form component to Ant Design 5 [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #32729:
URL: https://github.com/apache/superset/pull/32729#issuecomment-2734220370
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org