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: [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]
