-
Notifications
You must be signed in to change notification settings - Fork 13.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Upgrade Badge component to Ant Design 5 #29124
Conversation
superset-frontend/package.json
Outdated
"antd": "4.10.3", | ||
"antd-v5": "npm:antd@^5.18.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous and target versions of Ant Design can be supported by using different names in the package configuration.
@@ -22,7 +22,6 @@ import Badge from '.'; | |||
const mockedProps = { | |||
count: 9, | |||
text: 'Text', | |||
textColor: 'orange', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property is custom. Since it is not used in the code, it does not make sense to maintain it. Additionally, one of the goals of the proposal is to minimize the introduction of custom properties to reduce maintenance costs.
& > sup.ant-badge-count { | ||
${ | ||
count !== undefined | ||
? `background: ${color || theme.colors.primary.base};` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example of a necessary customization for backward compatibility. Ant Design uses colorError
as the background color for the count component of the Badge. However, in Superset, the count requires a different color. Changing the colorError
of the Badge in the Ant Design ConfigProvider
would affect all variations of the Badge, which is not desirable. See all variations here: https://ant.design/components/badge.
margin-left: ${theme.gridUnit * 2}px; | ||
&>sup { | ||
padding: 0 ${theme.gridUnit}px; | ||
&>sup.ant-badge-count { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is another example of a forced customization to meet Superset’s requirements to fit the Badge within the available space. Such customizations are undesirable as they introduce maintenance costs. Another issue to consider is the reliance on a specific className, which poses risks if this className is removed or changed in future versions of Ant Design.
@@ -35,35 +36,48 @@ const { common } = getBootstrapData(); | |||
|
|||
const extensionsRegistry = getExtensionsRegistry(); | |||
|
|||
const antdTheme = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mapping of the Ant Design theme with Superset theme
@@ -35,35 +36,48 @@ const { common } = getBootstrapData(); | |||
|
|||
const extensionsRegistry = getExtensionsRegistry(); | |||
|
|||
const antdTheme = { | |||
components: { | |||
Badge: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
55a7455
to
91582f3
Compare
9445618
to
26a4035
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Evan Rusackas <[email protected]>
SUMMARY
This was a Proof of Concept (POC) aiming to demonstrate the capability to upgrade Ant Design components to the latest version in isolation, facilitating incremental upgrades.
Two versions of Ant Design will be maintained until all components are upgraded to the latest version.
Please refer to the inline comments in this PR and the Superset Improvement Proposal #29268
This PR is dependent on PR #29328
Promoting this PR to a mergiable state.
BEFORE
AFTER
Note that the prop
textColor
has been removed as it was unused.TESTING INSTRUCTIONS
ADDITIONAL INFORMATION