geido commented on code in PR #32890:
URL: https://github.com/apache/superset/pull/32890#discussion_r2026881813
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,94 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { styled } from '@superset-ui/core';
-import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
+// eslint-disable-next-line
+import { Typography } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import Card, { CardProps } from 'src/components/Card';
import Icons from 'src/components/Icons';
-import LinesEllipsis from 'react-lines-ellipsis';
+import { SupersetTheme } from '@superset-ui/core';
-export interface IconButtonProps extends AntdButtonProps {
+export interface IconButtonProps extends CardProps {
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;
- }
- }
-
- &: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 IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ if (cardProps.onClick) {
+ cardProps.onClick(e as any);
+ }
+ if (e.key === ' ') {
+ e.preventDefault();
+ }
}
- }
-`;
-
-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 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;
- }
-`;
+ cardProps.onKeyDown?.(e);
+ };
-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>
+ const renderIcon = () => {
+ const iconContent = icon ? (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ css={(theme: SupersetTheme) => ({
Review Comment:
Agreed. Here is a reference to the frontend guidelines
https://github.com/apache/superset/wiki/Frontend-Style-Guidelines
##########
superset-frontend/src/components/IconButton/IconButton.stories.tsx:
##########
@@ -16,42 +16,37 @@
* specific language governing permissions and limitations
* under the License.
*/
-import IconButton, { IconButtonProps } from '.';
+import { Meta, StoryObj } from '@storybook/react';
+import IconButton from 'src/components/IconButton';
-export default {
- title: 'IconButton',
+const meta: Meta<typeof IconButton> = {
+ title: 'Components/IconButton',
component: IconButton,
+ argTypes: {
+ 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}
- />
-);
+export default meta;
-InteractiveIconButton.args = {
- buttonText: 'This is the IconButton text',
- altText: 'This is an example of non-default alt text',
- href: 'https://preset.io/',
- target: '_blank',
-};
+type Story = StoryObj<typeof IconButton>;
-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 Default: Story = {
+ args: {
+ buttonText: 'Default IconButton',
+ altText: 'Default icon button alt text',
},
};
+
+export const CustomIcon: Story = {
+ args: {
+ buttonText: 'Custom icon IconButton',
+ altText: 'Custom icon button alt text',
+ icon: '/images/sqlite.png'
+ },
+};
Review Comment:
@Sameerali0 you can install pre-commit or go with:
```
npm run prettier
npm run lint-fix
npm run type
```
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,94 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { styled } from '@superset-ui/core';
-import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
+// eslint-disable-next-line
+import { Typography } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import Card, { CardProps } from 'src/components/Card';
import Icons from 'src/components/Icons';
-import LinesEllipsis from 'react-lines-ellipsis';
+import { SupersetTheme } from '@superset-ui/core';
-export interface IconButtonProps extends AntdButtonProps {
+export interface IconButtonProps extends CardProps {
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;
- }
- }
-
- &: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 IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ if (cardProps.onClick) {
+ cardProps.onClick(e as any);
+ }
+ if (e.key === ' ') {
+ e.preventDefault();
+ }
}
- }
-`;
-
-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 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;
- }
-`;
+ cardProps.onKeyDown?.(e);
+ };
-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>
+ const renderIcon = () => {
+ const iconContent = icon ? (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ css={(theme: SupersetTheme) => ({
+ width: '100%',
+ height: '120px',
+ objectFit: 'contain',
+ })}
+ />
+ ) : (
+ <div
+ css={(theme: SupersetTheme) => ({
+ display: 'flex',
+ alignContent: 'center',
+ alignItems: 'center',
+ height: '120px',
+ })}
+ >
+ <Icons.DatabaseOutlined
+ css={(theme: SupersetTheme) => ({
+ fontSize: '48px',
+ })}
+ aria-label="default-icon"
+ />
+ </div>
+ );
- <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 iconContent;
+ };
- &: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
+ role="button"
+ tabIndex={0}
+ aria-label={buttonText}
+ onKeyDown={handleKeyDown}
+ cover={renderIcon()}
+ css={(theme: SupersetTheme) => ({
Review Comment:
Can we use the `css` interpolator with string syntax everywhere? Just avoid
the object as mentioned in the frontend guidelines.
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,109 @@
* 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 { useState } from 'react';
+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 IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const [isFocused, setIsFocused] = useState(false);
-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 handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ if (cardProps.onClick) {
+ cardProps.onClick(e as any);
+ }
+ if (e.key === ' ') {
+ e.preventDefault();
+ }
}
- }
-`;
+ cardProps.onKeyDown?.(e);
+ };
-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;
+ const renderIcon = () => {
+ const defaultIconStyle = {
+ fontSize: '48px',
+ color: 'var(--text-secondary)',
+ };
- &:first-of-type {
- margin-right: 0;
- }
-`;
+ const iconContent = icon ? (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ style={{
+ width: '100%',
+ height: '120px',
+ objectFit: 'contain',
+ }}
+ />
+ ) : (
+ <div
+ style={{
+ display: 'flex',
+ justifyContent: 'center',
+ alignItems: 'center',
+ height: '120px',
+ }}
+ >
+ <Icons.DatabaseOutlined
+ style={defaultIconStyle}
+ aria-label="default-icon"
Review Comment:
Icons currently do not accept a custom aria-label. I will fix it in a
separate PR.
##########
superset-frontend/src/components/IconButton/index.tsx:
##########
@@ -16,129 +16,94 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { styled } from '@superset-ui/core';
-import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
+// eslint-disable-next-line
+import { Typography } from 'src/components';
+import { Tooltip } from 'src/components/Tooltip';
+import Card, { CardProps } from 'src/components/Card';
import Icons from 'src/components/Icons';
-import LinesEllipsis from 'react-lines-ellipsis';
+import { SupersetTheme } from '@superset-ui/core';
-export interface IconButtonProps extends AntdButtonProps {
+export interface IconButtonProps extends CardProps {
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;
- }
- }
-
- &: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 IconButton: React.FC<IconButtonProps> = ({
+ buttonText,
+ icon,
+ altText,
+ ...cardProps
+}) => {
+ const handleKeyDown = (e: React.KeyboardEvent<HTMLDivElement>) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ if (cardProps.onClick) {
+ cardProps.onClick(e as any);
+ }
+ if (e.key === ' ') {
+ e.preventDefault();
+ }
}
- }
-`;
-
-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 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;
- }
-`;
+ cardProps.onKeyDown?.(e);
+ };
-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>
+ const renderIcon = () => {
+ const iconContent = icon ? (
+ <img
+ src={icon}
+ alt={altText || buttonText}
+ css={(theme: SupersetTheme) => ({
+ width: '100%',
+ height: '120px',
+ objectFit: 'contain',
+ })}
+ />
+ ) : (
+ <div
+ css={(theme: SupersetTheme) => ({
+ display: 'flex',
+ alignContent: 'center',
+ alignItems: 'center',
+ height: '120px',
+ })}
+ >
+ <Icons.DatabaseOutlined
+ css={(theme: SupersetTheme) => ({
+ fontSize: '48px',
Review Comment:
Can we enforce that the component always requires an icon instead of adding
a default here which seems pretty arbitrary? I think we might also want to just
accept Icons here from `src/components/Icons` instead of custom images if
possible.
--
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]
