korbit-ai[bot] commented on code in PR #32876:
URL: https://github.com/apache/superset/pull/32876#discussion_r2015825055
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,72 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { styled } from '@superset-ui/core';
-import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
+import { Card, Typography } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
-import LinesEllipsis from 'react-lines-ellipsis';
-export interface IconButtonProps extends AntdButtonProps {
+export interface IconButtonProps extends React.ComponentProps<typeof Card> {
buttonText: string;
icon: string;
altText?: string;
}
-const StyledButton = styled(Button)`
- height: auto;
- display: flex;
- flex-direction: column;
- padding: 0;
-`;
-
-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;
+const IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const renderIcon = () => {
+ if (icon) {
+ return (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ style={{
+ width: '100%',
+ height: '120px',
+ objectFit: 'contain',
+ }}
+ />
+ );
}
Review Comment:
### Memoize Icon Rendering Function <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The renderIcon function is recreated on every render due to being defined
inside the component, causing unnecessary memory allocations.
###### Why this matters
Creation of new function instances on each render can impact performance in
larger applications, especially when many IconButton components are rendered
simultaneously.
###### Suggested change ∙ *Feature Preview*
Move renderIcon outside the component or memoize it using useCallback:
```tsx
const IconButton: React.FC<IconButtonProps> = ({
buttonText,
icon,
altText,
...cardProps
}) => {
const renderIcon = useCallback(() => {
if (icon) {
return (
<img
src={icon}
alt={altText || buttonText}
style={{
width: '100%',
height: '120px',
objectFit: 'contain',
}}
/>
);
}
// ... rest of the function
}, [icon, altText, buttonText]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b1bc15d-41fb-4860-988e-7034308122f7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b1bc15d-41fb-4860-988e-7034308122f7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b1bc15d-41fb-4860-988e-7034308122f7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b1bc15d-41fb-4860-988e-7034308122f7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0b1bc15d-41fb-4860-988e-7034308122f7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6eff6427-361c-4a0d-bd6f-faf0bdbb1d63 -->
[](6eff6427-361c-4a0d-bd6f-faf0bdbb1d63)
##########
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,
+ ],
Review Comment:
### Hardcoded Icon Paths <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Hardcoded icon paths should be extracted into constants or an enum to
improve maintainability and readability.
###### Why this matters
Hardcoding paths across the codebase makes it difficult to update them
consistently and understand their purpose without additional context.
###### Suggested change ∙ *Feature Preview*
```typescript
const ICON_OPTIONS = {
SQL: '/images/icons/sql.svg',
SERVER: '/images/icons/server.svg',
IMAGE: '/images/icons/image.svg',
} as const;
// In options array:
options: [ICON_OPTIONS.SQL, ICON_OPTIONS.SERVER, ICON_OPTIONS.IMAGE, null],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecc75bc2-247f-4dcb-a8fd-5187f3b2ad0d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecc75bc2-247f-4dcb-a8fd-5187f3b2ad0d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecc75bc2-247f-4dcb-a8fd-5187f3b2ad0d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecc75bc2-247f-4dcb-a8fd-5187f3b2ad0d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ecc75bc2-247f-4dcb-a8fd-5187f3b2ad0d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:54feac76-0fcc-4cb5-9396-d116d2131812 -->
[](54feac76-0fcc-4cb5-9396-d116d2131812)
##########
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 behavior description <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Disabled story example doesn't explain how the disabled state affects
the component's behavior.
###### Why this matters
Developers need to understand the implications of the disabled state for
proper implementation.
###### Suggested change ∙ *Feature Preview*
export const Disabled: Story = {
args: {
// In disabled state, the button becomes non-interactive and visually
muted
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/6906daa4-b112-44e4-ab37-3918fc125a24/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6906daa4-b112-44e4-ab37-3918fc125a24?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6906daa4-b112-44e4-ab37-3918fc125a24?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6906daa4-b112-44e4-ab37-3918fc125a24?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6906daa4-b112-44e4-ab37-3918fc125a24)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:03239e8f-c6f4-4694-a670-279f3a465e7e -->
[](03239e8f-c6f4-4694-a670-279f3a465e7e)
##########
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 CustomOnClick story implementation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The CustomOnClick story doesn't demonstrate custom onClick behavior despite
its name.
###### Why this matters
The story misleads users by suggesting it shows custom click behavior, but
it only uses the default onClick action defined in meta.
###### Suggested change ∙ *Feature Preview*
Either rename the story to better reflect its current functionality or
implement a custom onClick handler:
```typescript
export const CustomOnClick: Story = {
args: {
buttonText: 'Clickable IconButton',
icon: '/images/icons/image.svg',
onClick: () => alert('Custom click handler executed'),
},
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/481d664a-83cb-4cf9-ac10-8415d8763e26/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/481d664a-83cb-4cf9-ac10-8415d8763e26?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/481d664a-83cb-4cf9-ac10-8415d8763e26?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/481d664a-83cb-4cf9-ac10-8415d8763e26?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/481d664a-83cb-4cf9-ac10-8415d8763e26)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:8abdc862-2646-4379-b1e6-f46e1dc27605 -->
[](8abdc862-2646-4379-b1e6-f46e1dc27605)
--
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]
