Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(Button): Upgrade Button component to Antd5 #31623

Merged
merged 8 commits into from
Jan 9, 2025
10 changes: 5 additions & 5 deletions superset-frontend/cypress-base/cypress/support/directories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export const databasesPage = {
alertMessage: '.antd5-alert-message',
errorField: '[role="alert"]',
uploadJson: '[title="Upload JSON file"]',
chooseFile: '[class="ant-btn input-upload-btn"]',
chooseFile: '[class="antd5-btn input-upload-btn"]',
additionalParameters: '[name="query_input"]',
sqlAlchemyUriInput: dataTestLocator('sqlalchemy-uri-input'),
advancedTab: '#rc-tabs-0-tab-2',
Expand Down Expand Up @@ -148,7 +148,7 @@ export const sqlLabView = {
examplesMenuItem: '[title="examples"]',
tableInput: ':nth-child(4) > .select > :nth-child(1)',
sqlEditor: '#brace-editor textarea',
saveAsButton: '.SaveQuery > .ant-btn',
saveAsButton: '.SaveQuery > .antd5-btn',
saveAsModal: {
footer: '.antd5-modal-footer',
queryNameInput: 'input[class^="ant-input"]',
Expand Down Expand Up @@ -195,7 +195,7 @@ export const savedQuery = {
export const annotationLayersView = {
emptyDescription: {
description: '.ant-empty-description',
addAnnotationLayerButton: '.ant-empty-footer > .ant-btn',
addAnnotationLayerButton: '.ant-empty-footer > .antd5-btn',
},
modal: {
content: {
Expand Down Expand Up @@ -434,7 +434,7 @@ export const dashboardListView = {
newDashboardButton: '.css-yff34v',
},
importModal: {
selectFileButton: '.ant-upload > .ant-btn > span',
selectFileButton: '.ant-upload > .antd5-btn > span',
importButton: dataTestLocator('modal-confirm-button'),
},
header: {
Expand Down Expand Up @@ -588,7 +588,7 @@ export const exploreView = {
rowsContainer: dataTestLocator('table-content-rows'),
},
confirmModal: {
okButton: '.antd5-modal-confirm-btns .ant-btn-primary',
okButton: '.antd5-modal-confirm-btns .antd5-btn-primary',
},
},
visualizationTypeModal: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const SaveDatasetActionButton = ({
const StyledDropdownButton = styled(
DropdownButton as FC<DropdownButtonProps>,
)`
&.ant-dropdown-button button.ant-btn.ant-btn-default {
&.ant-dropdown-button button.antd5-btn.antd5-btn-default {
font-weight: ${theme.gridUnit * 150};
background-color: ${theme.colors.primary.light4};
color: ${theme.colors.primary.dark1};
Expand Down
77 changes: 48 additions & 29 deletions superset-frontend/src/components/Button/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ import {

import { mix } from 'polished';
import cx from 'classnames';
import { Button as AntdButton } from 'antd';
import { Button as AntdButton } from 'antd-v5';
import { useTheme } from '@superset-ui/core';
import { Tooltip, TooltipProps } from 'src/components/Tooltip';
import { ButtonProps as AntdButtonProps } from 'antd/lib/button';
import { ButtonProps as AntdButtonProps } from 'antd-v5/lib/button';

export type OnClickHandler = MouseEventHandler<HTMLElement>;

Expand All @@ -56,6 +56,25 @@ export type ButtonProps = Omit<AntdButtonProps, 'css'> &
showMarginRight?: boolean;
};

const decideType = (buttonStyle: ButtonStyle) => {
const typeMap: Record<
ButtonStyle,
'primary' | 'default' | 'dashed' | 'link'
> = {
primary: 'primary',
danger: 'primary',
warning: 'primary',
success: 'primary',
secondary: 'default',
default: 'default',
tertiary: 'dashed',
dashed: 'dashed',
link: 'link',
};

return typeMap[buttonStyle];
};

export default function Button(props: ButtonProps) {
const {
tooltip,
Expand All @@ -73,7 +92,7 @@ export default function Button(props: ButtonProps) {

const theme = useTheme();
const { colors, transitionTiming, borderRadius, typography } = theme;
const { primary, grayscale, success, warning, error } = colors;
const { primary, grayscale, success, warning } = colors;

let height = 32;
let padding = 18;
Expand All @@ -85,25 +104,19 @@ export default function Button(props: ButtonProps) {
padding = 10;
}

let backgroundColor = primary.light4;
let backgroundColorHover = mix(0.1, primary.base, primary.light4);
let backgroundColorActive = mix(0.25, primary.base, primary.light4);
let backgroundColor;
let backgroundColorHover;
let backgroundColorActive;
let backgroundColorDisabled = grayscale.light2;
let color = primary.dark1;
let colorHover = color;
let color;
let colorHover;
let borderWidth = 0;
let borderStyle = 'none';
let borderColor = 'transparent';
let borderColorHover = 'transparent';
let borderColor;
let borderColorHover;
let borderColorDisabled = 'transparent';

if (buttonStyle === 'primary') {
backgroundColor = primary.base;
backgroundColorHover = primary.dark1;
backgroundColorActive = mix(0.2, grayscale.dark2, primary.dark1);
color = grayscale.light5;
colorHover = color;
} else if (buttonStyle === 'tertiary' || buttonStyle === 'dashed') {
if (buttonStyle === 'tertiary' || buttonStyle === 'dashed') {
backgroundColor = grayscale.light5;
backgroundColorHover = grayscale.light5;
backgroundColorActive = grayscale.light5;
Expand All @@ -114,10 +127,6 @@ export default function Button(props: ButtonProps) {
borderColorHover = primary.light1;
borderColorDisabled = grayscale.light2;
} else if (buttonStyle === 'danger') {
backgroundColor = error.base;
backgroundColorHover = mix(0.1, grayscale.light5, error.base);
backgroundColorActive = mix(0.2, grayscale.dark2, error.base);
color = grayscale.light5;
colorHover = color;
} else if (buttonStyle === 'warning') {
backgroundColor = warning.base;
Expand All @@ -135,7 +144,7 @@ export default function Button(props: ButtonProps) {
backgroundColor = 'transparent';
backgroundColorHover = 'transparent';
backgroundColorActive = 'transparent';
colorHover = primary.base;
color = primary.dark1;
}

const element = children as ReactElement;
Expand All @@ -149,10 +158,14 @@ export default function Button(props: ButtonProps) {
const firstChildMargin =
showMarginRight && renderedChildren.length > 1 ? theme.gridUnit * 2 : 0;

const effectiveButtonStyle: ButtonStyle = buttonStyle ?? 'default';

const button = (
<AntdButton
href={disabled ? undefined : href}
disabled={disabled}
type={decideType(effectiveButtonStyle)}
danger={effectiveButtonStyle === 'danger'}
className={cx(
className,
'superset-button',
Expand All @@ -179,12 +192,18 @@ export default function Button(props: ButtonProps) {
borderColor,
borderRadius,
color,
backgroundColor,
'&:hover': {
color: colorHover,
backgroundColor: backgroundColorHover,
borderColor: borderColorHover,
},
background: backgroundColor,
...(colorHover || backgroundColorHover || borderColorHover
? {
[`&.superset-button.superset-button-${buttonStyle}:hover`]: {
...(colorHover && { color: colorHover }),
...(backgroundColorHover && {
background: backgroundColorHover,
}),
...(borderColorHover && { borderColor: borderColorHover }),
},
}
: {}),
'&:active': {
color,
backgroundColor: backgroundColorActive,
Expand All @@ -206,7 +225,7 @@ export default function Button(props: ButtonProps) {
'& + .superset-button': {
marginLeft: theme.gridUnit * 2,
},
'& > :first-of-type': {
'& > span > :first-of-type': {
marginRight: firstChildMargin,
},
}}
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/components/DropdownButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ import { styled } from '@superset-ui/core';
import { kebabCase } from 'lodash';

const StyledDropdownButton = styled.div`
.ant-btn-group {
button.ant-btn {
.antd5-btn-group {
button.antd5-btn {
background-color: ${({ theme }) => theme.colors.primary.dark1};
border-color: transparent;
color: ${({ theme }) => theme.colors.grayscale.light5};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ export interface DropDownSelectableProps extends Pick<MenuProps, 'onSelect'> {
}

const StyledDropdownButton = styled(DropdownButton as FC<DropdownButtonProps>)`
button.ant-btn:first-of-type {
button.antd5-btn:first-of-type {
display: none;
}
> button.ant-btn:nth-of-type(2) {
> button.antd5-btn:nth-of-type(2) {
Comment on lines 43 to +47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invalid Ant Design v5 Button Class Names category Functionality

Tell me more
What is the issue?

The CSS selector 'antd5-btn' is incorrect. Ant Design v5 uses the class name 'ant-btn' for buttons, not 'antd5-btn'.

Why this matters

The styled components won't properly target and style the buttons because the CSS selectors are targeting non-existent classes, breaking the intended button styling functionality.

Suggested change

Replace 'antd5-btn' with 'ant-btn' in both CSS selectors:

button.ant-btn:first-of-type {
    display: none;
  }
  > button.ant-btn:nth-of-type(2) {
Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help Korbit improve your reviews.

display: inline-flex;
background-color: transparent !important;
height: unset;
Expand Down
3 changes: 1 addition & 2 deletions superset-frontend/src/components/IconButton/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
* under the License.
*/
import { styled } from '@superset-ui/core';
import Button from 'src/components/Button';
import { ButtonProps as AntdButtonProps } from 'antd/lib/button';
import Button, { ButtonProps as AntdButtonProps } from 'src/components/Button';
import Icons from 'src/components/Icons';
import LinesEllipsis from 'react-lines-ellipsis';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export const menuTriggerStyles = (theme: SupersetTheme) => css`
padding: 0;
border: 1px solid ${theme.colors.primary.dark2};

&.ant-btn > span.anticon {
&.antd5-btn > span.anticon {
line-height: 0;
transition: inherit;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const AddButtonContainer = styled.div`
padding-bottom: 1px;
}

.ant-btn > .anticon + span {
.antd5-btn > .anticon + span {
margin-left: 0;
}
`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ export const MenuTrigger = styled(Button)`
padding: 0;
border: 1px solid ${theme.colors.primary.dark2};

&.ant-btn > span.anticon {
&.antd5-btn > span.anticon {
line-height: 0;
transition: inherit;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ describe('DatabaseModal', () => {
];

visibleComponents.forEach(component => {
expect(component).toBeVisible();
expect(component).toBeInTheDocument();
});
});

Expand Down Expand Up @@ -781,7 +781,7 @@ describe('DatabaseModal', () => {
enableRowExpansionCheckbox,
];
visibleComponents.forEach(component => {
expect(component).toBeVisible();
expect(component).toBeInTheDocument();
});
invisibleComponents.forEach(component => {
expect(component).not.toBeVisible();
Expand Down Expand Up @@ -849,7 +849,7 @@ describe('DatabaseModal', () => {
];

visibleComponents.forEach(component => {
expect(component).toBeVisible();
expect(component).toBeInTheDocument();
});
});

Expand Down Expand Up @@ -929,7 +929,7 @@ describe('DatabaseModal', () => {

// ---------- Assertions ----------
visibleComponents.forEach(component => {
expect(component).toBeVisible();
expect(component).toBeInTheDocument();
});
invisibleComponents.forEach(component => {
expect(component).not.toBeVisible();
Expand Down Expand Up @@ -1137,7 +1137,7 @@ describe('DatabaseModal', () => {
expect(await screen.findByText(/step 2 of 2/i)).toBeInTheDocument();
const sqlAlchemyFormStepText = screen.getByText(/step 2 of 2/i);

expect(sqlAlchemyFormStepText).toBeVisible();
expect(sqlAlchemyFormStepText).toBeInTheDocument();
});

describe('SQL Alchemy form flow', () => {
Expand Down Expand Up @@ -1293,7 +1293,7 @@ describe('DatabaseModal', () => {

expect(await screen.findByText(/step 2 of 2/i)).toBeInTheDocument();
const SSHTunnelingToggle = screen.getByTestId('ssh-tunnel-switch');
expect(SSHTunnelingToggle).toBeVisible();
expect(SSHTunnelingToggle).toBeInTheDocument();
const SSHTunnelServerAddressInput = screen.queryByTestId(
'ssh-tunnel-server_address-input',
);
Expand Down Expand Up @@ -1325,26 +1325,26 @@ describe('DatabaseModal', () => {
const SSHTunnelUsePasswordInput = screen.getByTestId(
'ssh-tunnel-use_password-radio',
);
expect(SSHTunnelUsePasswordInput).toBeVisible();
expect(SSHTunnelUsePasswordInput).toBeInTheDocument();
const SSHTunnelUsePrivateKeyInput = screen.getByTestId(
'ssh-tunnel-use_private_key-radio',
);
expect(SSHTunnelUsePrivateKeyInput).toBeVisible();
expect(SSHTunnelUsePrivateKeyInput).toBeInTheDocument();
const SSHTunnelPasswordInput = screen.getByTestId(
'ssh-tunnel-password-input',
);
// By default, we use Password as login method
expect(SSHTunnelPasswordInput).toBeVisible();
expect(SSHTunnelPasswordInput).toBeInTheDocument();
// Change the login method to use private key
userEvent.click(SSHTunnelUsePrivateKeyInput);
const SSHTunnelPrivateKeyInput = screen.getByTestId(
'ssh-tunnel-private_key-input',
);
expect(SSHTunnelPrivateKeyInput).toBeVisible();
expect(SSHTunnelPrivateKeyInput).toBeInTheDocument();
const SSHTunnelPrivateKeyPasswordInput = screen.getByTestId(
'ssh-tunnel-private_key_password-input',
);
expect(SSHTunnelPrivateKeyPasswordInput).toBeVisible();
expect(SSHTunnelPrivateKeyPasswordInput).toBeInTheDocument();
});
});
});
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/features/datasets/styles.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ export const StyledLayoutFooter = styled.div`
`;

export const HeaderComponentStyles = styled.div`
.ant-btn {
.antd5-btn {
span {
margin-right: 0;
}
Expand Down
Loading
Loading