msyavuz commented on code in PR #32890:
URL: https://github.com/apache/superset/pull/32890#discussion_r2021545776
##########
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"
+ />
+ </div>
+ );
-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>
+ return iconContent;
+ };
- <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%;
+ const focusStyles = isFocused
+ ? {
+ border: '2px solid #1890ff',
+ boxShadow: '0 0 0 3px rgba(24, 144, 255, 0.1)',
+ }
+ : {};
- &: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}
+ onFocus={e => {
+ cardProps.onFocus?.(e);
+ setIsFocused(true);
+ }}
+ onBlur={e => {
+ cardProps.onBlur?.(e);
+ setIsFocused(false);
+ }}
+ {...cardProps}
Review Comment:
We can move this to the end of props to let usage override `cover` prop as
well
##########
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);
Review Comment:
Using `css` might be better for this
##########
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={{
Review Comment:
Let's use `css` prop or styled components instead of `style` prop. You can
see examples in other components
##########
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"
+ />
+ </div>
+ );
-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>
+ return iconContent;
+ };
- <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%;
+ const focusStyles = isFocused
+ ? {
+ border: '2px solid #1890ff',
+ boxShadow: '0 0 0 3px rgba(24, 144, 255, 0.1)',
Review Comment:
Let's use theme color variables instead of hex/rgb(a).We have eslint rules
to catch these, I recommend setting up eslint in your editor.
##########
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> {
Review Comment:
There should be `CardProps` exported from `src/components/Card`
##########
superset-frontend/src/components/IconButton/IconButton.test.jsx:
##########
@@ -25,13 +25,81 @@ const defaultProps = {
};
describe('IconButton', () => {
- it('renders an IconButton', () => {
+ it('renders an IconButton with icon and text', () => {
render(<IconButton {...defaultProps} />);
-
+
const icon = screen.getByRole('img');
const buttonText = screen.getByText(/this is the iconbutton text/i);
-
+
expect(icon).toBeVisible();
expect(buttonText).toBeVisible();
});
-});
+
+ it('is keyboard accessible and has correct aria attributes', () => {
+ render(<IconButton {...defaultProps} />);
+
+ const button = screen.getByRole('button');
+
+ expect(button).toHaveAttribute('tabIndex', '0');
+ expect(button).toHaveAttribute('aria-label', defaultProps.buttonText);
+ });
+
+ it('handles Enter and Space key presses', () => {
+ const mockOnClick = jest.fn();
+ render(<IconButton {...defaultProps} onClick={mockOnClick} />);
+
+ const button = screen.getByRole('button');
+
+ fireEvent.keyDown(button, { key: 'Enter', code: 'Enter' });
+ expect(mockOnClick).toHaveBeenCalledTimes(1);
+
+ fireEvent.keyDown(button, { key: ' ', code: 'Space' });
+ expect(mockOnClick).toHaveBeenCalledTimes(2);
+ });
+
+ it('renders default icon when no icon is provided', () => {
+ render(<IconButton buttonText="No Icon Button" />);
Review Comment:
Can we convert this file to `tsx` as well? For example here we are testing
if `icon` is not provided but `icon` is a required prop.
--
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]
