You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "fardin-developer (via GitHub)" <gi...@apache.org> on 2025/03/27 06:06:06 UTC
[PR] refactor(IconButton): Refactor IconButton to use Ant Design 5 Card [superset]
fardin-developer opened a new pull request, #32875:
URL: https://github.com/apache/superset/pull/32875
Fixes: #32823
### SUMMARY
This PR removes the non-standard `IconButton` component and replaces it with the standard `Card` component from Ant Design 5, as part of the migration effort. The component is currently used in the Database modal.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### CHANGES
- Removed `src/components/IconButton` component.
- Updated all instances to use Ant Design 5's `Card` component instead.
- Ensured proper styling with minimal custom overrides.
- Maintained RTL support and existing behavior.
### BEFORE/AFTER SCREENSHOTS
#### Before
#### After
### ADDITIONAL NOTES
- Ensured Storybook coverage for `Card` usage.
- Verified that the component works correctly in all relevant UI flows.
### CHECKLIST
- [x] Used Ant Design 5 `Card` component.
- [x] Removed unnecessary custom styles.
- [x] Added Storybook documentation.
- [x] Verified RTL compatibility.
- [x] Labeled the PR with `frontend:refactor:antd5`.
--
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(IconButton): Refactor IconButton to use Ant Design 5 Card [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #32875:
URL: https://github.com/apache/superset/pull/32875#discussion_r2015722197
##########
superset-frontend/src/components/IconButton/IconButton.stories.tsx:
##########
@@ -16,42 +16,69 @@
* specific language governing permissions and limitations
* under the License.
*/
-import IconButton, { IconButtonProps } from '.';
+import { Meta, StoryObj } from '@storybook/react';
+import IconButton from '.';
-export default {
- title: 'IconButton',
+const meta: Meta<typeof IconButton> = {
+ title: 'Components/IconButton',
component: IconButton,
+ argTypes: {
+ icon: {
+ control: {
+ type: 'select',
+ options: [
+ '/images/icons/sql.svg',
+ '/images/icons/server.svg',
+ '/images/icons/image.svg',
+ null,
+ ],
+ },
+ },
+ onClick: { action: 'clicked' },
+ },
+ parameters: {
+ a11y: {
+ enabled: true,
+ },
+ },
};
-export const InteractiveIconButton = (args: IconButtonProps) => (
- <IconButton
- buttonText={args.buttonText}
- altText={args.altText}
- icon={args.icon}
- href={args.href}
- target={args.target}
- htmlType={args.htmlType}
- />
-);
-
-InteractiveIconButton.args = {
- buttonText: 'This is the IconButton text',
- altText: 'This is an example of non-default alt text',
- href: 'https://preset.io/',
- target: '_blank',
+export default meta;
+
+type Story = StoryObj<typeof IconButton>;
+
+export const Default: Story = {
+ args: {
+ buttonText: 'Default IconButton',
+ altText: 'Default icon button',
+ icon: '/images/icons/sql.svg',
+ },
};
-InteractiveIconButton.argTypes = {
- icon: {
- defaultValue: '/images/icons/sql.svg',
- control: {
- type: 'select',
- },
- options: [
- '/images/icons/sql.svg',
- '/images/icons/server.svg',
- '/images/icons/image.svg',
- 'Click to see example alt text',
- ],
+export const WithoutIcon: Story = {
+ args: {
+ buttonText: 'IconButton without custom icon',
+ },
+};
+
+export const LongText: Story = {
+ args: {
+ buttonText:
+ 'This is a very long button text that will be truncated with ellipsis to show multiline behavior',
+ icon: '/images/icons/server.svg',
+ },
+};
+
+export const CustomOnClick: Story = {
+ args: {
+ buttonText: 'Clickable IconButton',
+ icon: '/images/icons/image.svg',
+ },
+};
+
+export const Disabled: Story = {
+ args: {
+ buttonText: 'Disabled IconButton',
+ icon: '/images/icons/sql.svg',
},
};
Review Comment:
### Missing Disabled State Implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The 'Disabled' story is missing the 'disabled' prop despite being meant to showcase a disabled state.
###### Why this matters
Users of the Storybook documentation won't be able to see or test the disabled state functionality of the IconButton component.
###### Suggested change ∙ *Feature Preview*
Add the disabled prop to the Disabled story:
```typescript
export const Disabled: Story = {
args: {
buttonText: 'Disabled IconButton',
icon: '/images/icons/sql.svg',
disabled: true,
},
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e647d0e-4d10-4b28-832f-a31bfcdaf340/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e647d0e-4d10-4b28-832f-a31bfcdaf340?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e647d0e-4d10-4b28-832f-a31bfcdaf340?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e647d0e-4d10-4b28-832f-a31bfcdaf340?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/1e647d0e-4d10-4b28-832f-a31bfcdaf340)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:27a411dd-3866-46e7-93e3-308705bec283 -->
[](27a411dd-3866-46e7-93e3-308705bec283)
##########
superset-frontend/src/components/IconButton/IconButton.stories.tsx:
##########
@@ -16,42 +16,69 @@
* specific language governing permissions and limitations
* under the License.
*/
-import IconButton, { IconButtonProps } from '.';
+import { Meta, StoryObj } from '@storybook/react';
+import IconButton from '.';
-export default {
- title: 'IconButton',
+const meta: Meta<typeof IconButton> = {
+ title: 'Components/IconButton',
component: IconButton,
+ argTypes: {
+ icon: {
+ control: {
+ type: 'select',
+ options: [
+ '/images/icons/sql.svg',
+ '/images/icons/server.svg',
+ '/images/icons/image.svg',
Review Comment:
### Hardcoded Icon Paths <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Icon paths are hardcoded strings repeated multiple times throughout the stories.
###### Why this matters
Hardcoded strings make maintenance difficult and prone to errors when paths need to be updated. Changes would need to be made in multiple places.
###### Suggested change ∙ *Feature Preview*
```typescript
const ICON_PATHS = {
SQL: '/images/icons/sql.svg',
SERVER: '/images/icons/server.svg',
IMAGE: '/images/icons/image.svg',
} as const;
// Use in options and stories
options: Object.values(ICON_PATHS),
// ...
icon: ICON_PATHS.SQL,
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a8203fe-e9ec-44db-97eb-21bd6eba6577/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a8203fe-e9ec-44db-97eb-21bd6eba6577?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a8203fe-e9ec-44db-97eb-21bd6eba6577?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a8203fe-e9ec-44db-97eb-21bd6eba6577?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/9a8203fe-e9ec-44db-97eb-21bd6eba6577)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8fa90520-2799-4186-a5ef-9d6225ac2737 -->
[](8fa90520-2799-4186-a5ef-9d6225ac2737)
##########
superset-frontend/src/components/IconButton/IconButton.stories.tsx:
##########
@@ -16,42 +16,69 @@
* specific language governing permissions and limitations
* under the License.
*/
-import IconButton, { IconButtonProps } from '.';
+import { Meta, StoryObj } from '@storybook/react';
+import IconButton from '.';
-export default {
- title: 'IconButton',
+const meta: Meta<typeof IconButton> = {
+ title: 'Components/IconButton',
component: IconButton,
+ argTypes: {
+ icon: {
+ control: {
+ type: 'select',
+ options: [
+ '/images/icons/sql.svg',
+ '/images/icons/server.svg',
+ '/images/icons/image.svg',
+ null,
+ ],
+ },
+ },
+ onClick: { action: 'clicked' },
+ },
+ parameters: {
+ a11y: {
+ enabled: true,
+ },
+ },
};
-export const InteractiveIconButton = (args: IconButtonProps) => (
- <IconButton
- buttonText={args.buttonText}
- altText={args.altText}
- icon={args.icon}
- href={args.href}
- target={args.target}
- htmlType={args.htmlType}
- />
-);
-
-InteractiveIconButton.args = {
- buttonText: 'This is the IconButton text',
- altText: 'This is an example of non-default alt text',
- href: 'https://preset.io/',
- target: '_blank',
+export default meta;
+
+type Story = StoryObj<typeof IconButton>;
+
+export const Default: Story = {
+ args: {
+ buttonText: 'Default IconButton',
+ altText: 'Default icon button',
+ icon: '/images/icons/sql.svg',
+ },
};
-InteractiveIconButton.argTypes = {
- icon: {
- defaultValue: '/images/icons/sql.svg',
- control: {
- type: 'select',
- },
- options: [
- '/images/icons/sql.svg',
- '/images/icons/server.svg',
- '/images/icons/image.svg',
- 'Click to see example alt text',
- ],
+export const WithoutIcon: Story = {
+ args: {
+ buttonText: 'IconButton without custom icon',
+ },
+};
+
+export const LongText: Story = {
+ args: {
+ buttonText:
+ 'This is a very long button text that will be truncated with ellipsis to show multiline behavior',
+ icon: '/images/icons/server.svg',
+ },
+};
+
+export const CustomOnClick: Story = {
+ args: {
+ buttonText: 'Clickable IconButton',
+ icon: '/images/icons/image.svg',
+ },
+};
Review Comment:
### Incomplete Click Handler Story <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The CustomOnClick story doesn't demonstrate the onClick functionality despite its name suggesting it should.
###### Why this matters
While onClick is defined in argTypes, the story itself doesn't showcase how to implement a custom click handler.
###### Suggested change ∙ *Feature Preview*
Add an onClick handler to the CustomOnClick story:
```typescript
export const CustomOnClick: Story = {
args: {
buttonText: 'Clickable IconButton',
icon: '/images/icons/image.svg',
onClick: () => alert('Button clicked!'),
},
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59627c36-5d84-4b7d-bf5c-2ef762e1ada9/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59627c36-5d84-4b7d-bf5c-2ef762e1ada9?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59627c36-5d84-4b7d-bf5c-2ef762e1ada9?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59627c36-5d84-4b7d-bf5c-2ef762e1ada9?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/59627c36-5d84-4b7d-bf5c-2ef762e1ada9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4c13afca-6c2e-4254-aa05-f28c9de712bc -->
[](4c13afca-6c2e-4254-aa05-f28c9de712bc)
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { styled } from '@superset-ui/core';
-import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
+// src/components/IconButton/index.tsx
+import { Card, Typography, Tooltip } from 'antd';
+import { CardProps } from 'antd/lib/card';
import Icons from 'src/components/Icons';
-import LinesEllipsis from 'react-lines-ellipsis';
-export interface IconButtonProps extends AntdButtonProps {
+export interface IconButtonProps extends CardProps {
buttonText: string;
- icon: string;
+ icon?: string;
altText?: string;
}
-const StyledButton = styled(Button)`
- height: auto;
- display: flex;
- flex-direction: column;
- padding: 0;
-`;
+const IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const { Text } = Typography;
-const StyledImage = styled.div`
- padding: ${({ theme }) => theme.gridUnit * 4}px;
- height: ${({ theme }) => theme.gridUnit * 18}px;
- margin: ${({ theme }) => theme.gridUnit * 3}px 0;
-
- .default-db-icon {
- font-size: 36px;
- color: ${({ theme }) => theme.colors.grayscale.base};
- margin-right: 0;
- span:first-of-type {
- margin-right: 0;
- }
- }
-
- &:first-of-type {
- margin-right: 0;
- }
-
- img {
- width: ${({ theme }) => theme.gridUnit * 10}px;
- height: ${({ theme }) => theme.gridUnit * 10}px;
- margin: 0;
- &:first-of-type {
- margin-right: 0;
- }
- }
- svg {
- &:first-of-type {
- margin-right: 0;
- }
- }
-`;
-
-const StyledInner = styled.div`
- max-height: calc(1.5em * 2);
- white-space: break-spaces;
-
- &:first-of-type {
- margin-right: 0;
- }
-
- .LinesEllipsis {
- &:first-of-type {
- margin-right: 0;
+ const renderIcon = () => {
+ if (icon) {
+ return (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ style={{ width: '100%', height: '120px', objectFit: 'contain' }}
+ />
+ );
}
- }
-`;
-
-const StyledBottom = styled.div`
- padding: ${({ theme }) => theme.gridUnit * 4}px 0;
- border-radius: 0 0 ${({ theme }) => theme.borderRadius}px
- ${({ theme }) => theme.borderRadius}px;
- background-color: ${({ theme }) => theme.colors.grayscale.light4};
- width: 100%;
- line-height: 1.5em;
- overflow: hidden;
- white-space: no-wrap;
- text-overflow: ellipsis;
-
- &:first-of-type {
- margin-right: 0;
- }
-`;
-
-const IconButton = styled(
- ({ icon, altText, buttonText, ...props }: IconButtonProps) => (
- <StyledButton {...props}>
- <StyledImage>
- {icon && <img src={icon} alt={altText} />}
- {!icon && (
- <Icons.DatabaseOutlined
- className="default-db-icon"
- aria-label="default-icon"
- />
- )}
- </StyledImage>
- <StyledBottom>
- <StyledInner>
- <LinesEllipsis
- text={buttonText}
- maxLine="2"
- basedOn="words"
- trimRight
- />
- </StyledInner>
- </StyledBottom>
- </StyledButton>
- ),
-)`
- text-transform: none;
- background-color: ${({ theme }) => theme.colors.grayscale.light5};
- font-weight: ${({ theme }) => theme.typography.weights.normal};
- color: ${({ theme }) => theme.colors.grayscale.dark2};
- border: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
- margin: 0;
- width: 100%;
+ return (
+ <div
+ style={{
+ display: 'flex',
+ justifyContent: 'center',
+ alignItems: 'center',
+ height: '120px',
+ }}
+ >
+ <Icons.DatabaseOutlined
+ style={{ fontSize: '48px', color: 'var(--text-secondary)' }}
+ aria-label="default-icon"
+ />
+ </div>
+ );
+ };
- &:hover,
- &:focus {
- background-color: ${({ theme }) => theme.colors.grayscale.light5};
- color: ${({ theme }) => theme.colors.grayscale.dark2};
- border: 1px solid ${({ theme }) => theme.colors.grayscale.light2};
- box-shadow: 4px 4px 20px ${({ theme }) => theme.colors.grayscale.light2};
- }
-`;
+ return (
+ <Card
+ hoverable
+ {...cardProps}
+ cover={renderIcon()}
+ bodyStyle={{ padding: '12px', textAlign: 'center' }}
Review Comment:
### Inline Styles Instead of Styled Components <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Inline styles are used for Card body styling instead of a more maintainable styling approach
###### Why this matters
Inline styles make it harder to maintain consistent styling patterns across the application and reduce code reusability.
###### Suggested change ∙ *Feature Preview*
Use styled-components or a CSS module:
```typescript
const StyledCard = styled(Card)`
.ant-card-body {
padding: ${({ theme }) => theme.gridUnit * 3}px;
text-align: center;
}
`;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d8118a6-b7f3-4d73-ac0b-b483cdecc567/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d8118a6-b7f3-4d73-ac0b-b483cdecc567?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d8118a6-b7f3-4d73-ac0b-b483cdecc567?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d8118a6-b7f3-4d73-ac0b-b483cdecc567?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2d8118a6-b7f3-4d73-ac0b-b483cdecc567)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d3857cb8-6940-4c68-bda6-a6ec6081aa44 -->
[](d3857cb8-6940-4c68-bda6-a6ec6081aa44)
##########
superset-frontend/src/components/IconButton/IconButton.stories.tsx:
##########
@@ -16,42 +16,69 @@
* specific language governing permissions and limitations
* under the License.
*/
-import IconButton, { IconButtonProps } from '.';
+import { Meta, StoryObj } from '@storybook/react';
+import IconButton from '.';
-export default {
- title: 'IconButton',
+const meta: Meta<typeof IconButton> = {
+ title: 'Components/IconButton',
component: IconButton,
+ argTypes: {
+ icon: {
+ control: {
+ type: 'select',
+ options: [
+ '/images/icons/sql.svg',
+ '/images/icons/server.svg',
+ '/images/icons/image.svg',
+ null,
+ ],
+ },
+ },
+ onClick: { action: 'clicked' },
+ },
+ parameters: {
+ a11y: {
+ enabled: true,
+ },
+ },
};
Review Comment:
### Missing Storybook component description <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Missing description in the meta object to explain the purpose and usage of the IconButton component in Storybook.
###### Why this matters
Without a component description, other developers won't understand the intended use cases and limitations of the IconButton component.
###### Suggested change ∙ *Feature Preview*
const meta: Meta<typeof IconButton> = {
title: 'Components/IconButton',
component: IconButton,
parameters: {
docs: {
description: {
component: 'A reusable button component that combines an icon with text. Supports accessibility features and various states like disabled.'
}
},
a11y: {
enabled: true
}
},
argTypes: {...}
};
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f74bcae-8736-47b0-bb49-2cdb91d79518/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f74bcae-8736-47b0-bb49-2cdb91d79518?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f74bcae-8736-47b0-bb49-2cdb91d79518?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f74bcae-8736-47b0-bb49-2cdb91d79518?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6f74bcae-8736-47b0-bb49-2cdb91d79518)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5dd5d8b3-6b7c-4dd4-bdca-29e645ee80c6 -->
[](5dd5d8b3-6b7c-4dd4-bdca-29e645ee80c6)
--
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(IconButton): Refactor IconButton to use Ant Design 5 Card [superset]
Posted by "fardin-developer (via GitHub)" <gi...@apache.org>.
fardin-developer closed pull request #32875: refactor(IconButton): Refactor IconButton to use Ant Design 5 Card
URL: https://github.com/apache/superset/pull/32875
--
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