Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions superset-frontend/packages/superset-core/src/theme/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,17 @@ export interface SupersetSpecificTokens {
* Fallback: transparent
*/
buttonSecondaryActiveBorderColor?: string;

// Component flexibility tokens (sizing, radius, behavior)
selectOptionActiveOutline?: boolean;
labelBorderRadius?: number;
buttonControlHeight?: number;
buttonControlHeightSM?: number;
buttonControlHeightXS?: number;
buttonPaddingInline?: number;
buttonPaddingInlineSM?: number;
buttonFontSize?: number;
buttonBorderRadius?: number;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ InteractiveButton.argTypes = {
options: buttonSizes,
control: { type: 'select' },
},
styleConfig: {
description:
'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap).',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing borderRadius in description

The styleConfig description lists 7 properties but ButtonStyleConfig (types.ts:43-52) includes 8 properties — borderRadius is missing. Users relying on Storybook controls will not know this option exists.

Code suggestion
Check the AI-generated fix before applying
 --- superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx
 +++ superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx
 @@ -128,7 +128,7 @@ InteractiveButton.argTypes = {
    styleConfig: {
      description:
 -      'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap).',
 +      'Optional visual overrides (controlHeight, paddingInline, fontSize, fontWeight, ctaMinWidth, ctaMinHeight, iconGap, borderRadius).',
      control: { type: 'object' },
    },

Code Review Run #03fc85


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

control: { type: 'object' },
},
target: {
name: TARGETS.label,
control: { type: 'select' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import type { SupersetTheme } from '@apache-superset/core/theme';
import type {
ButtonColorType,
ButtonProps,
ButtonStyleConfig,
ButtonStyle,
ButtonType,
ButtonVariantType,
Expand Down Expand Up @@ -84,14 +85,13 @@ export const getSecondaryButtonHoverStyles = (theme: SupersetTheme) => ({
},
});

const BUTTON_STYLE_MAP: Record<
ButtonStyle,
{
type?: ButtonType;
variant?: ButtonVariantType;
color?: ButtonColorType;
}
> = {
type ButtonStyleMapping = {
type?: ButtonType;
variant?: ButtonVariantType;
color?: ButtonColorType;
};

const BUTTON_STYLE_MAP: Record<ButtonStyle, ButtonStyleMapping> = {
primary: { type: 'primary', variant: 'solid', color: 'primary' },
secondary: { variant: 'filled', color: 'primary' },
tertiary: { variant: 'outlined', color: 'default' },
Expand All @@ -113,30 +113,59 @@ export function Button(props: ButtonProps) {
href,
showMarginRight = true,
icon,
styleConfig,
...restProps
} = props;

const theme = useTheme();
const { fontSizeSM, fontWeightStrong } = theme;
const { fontWeightStrong } = theme;
const btnFontSize = theme.buttonFontSize ?? theme.fontSizeSM;

const themeExt = theme as typeof theme & {
buttonStyleMap?: Partial<Record<ButtonStyle, Partial<ButtonStyleMapping>>>;
};
const resolvedStyleMap: Record<ButtonStyle, ButtonStyleMapping> =
themeExt.buttonStyleMap
? (Object.fromEntries(
Object.entries(BUTTON_STYLE_MAP).map(([key, value]) => [
key,
{ ...value, ...themeExt.buttonStyleMap?.[key as ButtonStyle] },
]),
) as Record<ButtonStyle, ButtonStyleMapping>)
: BUTTON_STYLE_MAP;

let height = 32;
let padding = 18;
let defaultHeight = theme.buttonControlHeight ?? 32;
let defaultPaddingInline = theme.buttonPaddingInline ?? 18;
if (buttonSize === 'xsmall') {
height = 22;
padding = 5;
defaultHeight = theme.buttonControlHeightXS ?? 22;
defaultPaddingInline = 5;
} else if (buttonSize === 'small') {
height = 30;
padding = 10;
defaultHeight = theme.buttonControlHeightSM ?? 30;
defaultPaddingInline = theme.buttonPaddingInlineSM ?? 10;
}
if (buttonStyle === 'link') {
padding = 4;
defaultPaddingInline = 4;
}

const resolvedStyleConfig: Required<ButtonStyleConfig> = {
controlHeight: styleConfig?.controlHeight ?? defaultHeight,
paddingInline: styleConfig?.paddingInline ?? defaultPaddingInline,
fontSize: styleConfig?.fontSize ?? btnFontSize,
fontWeight: styleConfig?.fontWeight ?? fontWeightStrong,
ctaMinWidth: styleConfig?.ctaMinWidth ?? theme.sizeUnit * 36,
ctaMinHeight: styleConfig?.ctaMinHeight ?? theme.sizeUnit * 8,
iconGap: styleConfig?.iconGap ?? theme.sizeUnit * 2,
borderRadius:
styleConfig?.borderRadius ??
theme.buttonBorderRadius ??
theme.borderRadius,
};

const {
type: antdType = 'default',
variant,
color,
} = BUTTON_STYLE_MAP[buttonStyle ?? 'primary'];
} = resolvedStyleMap[buttonStyle ?? 'primary'] ?? BUTTON_STYLE_MAP.primary;

const element = children as ReactElement;

Expand All @@ -148,7 +177,9 @@ export function Button(props: ButtonProps) {
renderedChildren = Children.toArray(children);
}
const firstChildMargin =
showMarginRight && renderedChildren.length > 1 ? theme.sizeUnit * 2 : 0;
showMarginRight && renderedChildren.length > 1
? resolvedStyleConfig.iconGap
: 0;

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

Expand All @@ -169,8 +200,6 @@ export function Button(props: ButtonProps) {
className={cx(
className,
'superset-button',
// A static class name containing the button style is available to
// support customizing button styles in embedded dashboards.
`superset-button-${buttonStyle}`,
{ cta: !!cta },
)}
Expand All @@ -179,12 +208,13 @@ export function Button(props: ButtonProps) {
alignItems: 'center',
justifyContent: 'center',
lineHeight: 1,
fontSize: fontSizeSM,
fontWeight: fontWeightStrong,
height,
padding: `0px ${padding}px`,
minWidth: cta ? theme.sizeUnit * 36 : undefined,
minHeight: cta ? theme.sizeUnit * 8 : undefined,
fontSize: resolvedStyleConfig.fontSize,
fontWeight: resolvedStyleConfig.fontWeight,
height: resolvedStyleConfig.controlHeight,
padding: `0px ${resolvedStyleConfig.paddingInline}px`,
borderRadius: resolvedStyleConfig.borderRadius,
minWidth: cta ? resolvedStyleConfig.ctaMinWidth : undefined,
minHeight: cta ? resolvedStyleConfig.ctaMinHeight : undefined,
marginLeft: 0,
'& + .superset-button:not(.ant-btn-compact-item)': {
marginLeft: theme.sizeUnit * 2,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ export type ButtonStyle =

export type ButtonSize = 'default' | 'small' | 'xsmall';

export type ButtonStyleConfig = {
controlHeight?: number;
paddingInline?: number;
fontSize?: number;
fontWeight?: number;
ctaMinWidth?: number;
ctaMinHeight?: number;
iconGap?: number;
borderRadius?: number;
};

export type ButtonProps = Omit<AntdButtonProps, 'css'> & {
placement?: TooltipPlacement;
tooltip?: ReactNode;
Expand All @@ -49,4 +60,5 @@ export type ButtonProps = Omit<AntdButtonProps, 'css'> & {
cta?: boolean;
showMarginRight?: boolean;
icon?: ReactNode;
styleConfig?: ButtonStyleConfig;
};
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ InteractiveDropdownButton.args = {
};

InteractiveDropdownButton.argTypes = {
styleConfig: {
description:
'Optional visual overrides (controlHeight, fontSize, fontWeight, boxShadow).',
control: { type: 'object' },
},
placement: {
defaultValue: 'top',
control: { type: 'select' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const DropdownButton = ({
tooltip,
tooltipPlacement,
children,
styleConfig,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing unit tests for styleConfig

The new styleConfig prop lacks unit test coverage. Per rule [6262], tests should verify actual behavior logic — not just rendering. Add tests validating each styleConfig property (controlHeight, fontSize, fontWeight, boxShadow) and their defaults when the prop is omitted.

Code Review Run #03fc85


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

...rest
}: DropdownButtonProps) => {
const theme = useTheme();
Expand Down Expand Up @@ -57,10 +58,10 @@ export const DropdownButton = ({
defaultBtnCss,
css`
.ant-btn {
height: 30px;
box-shadow: none;
font-size: ${theme.fontSizeSM}px;
font-weight: ${theme.fontWeightStrong};
height: ${styleConfig?.controlHeight ?? 30}px;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: This hardcoded fallback height ignores the component's size prop and also diverges from the shared button token defaults, so default dropdown buttons render at 30px while regular default buttons render at 32px. That creates visible misalignment in mixed button rows (for example, the dataset footer) and breaks the expected AntD size contract. Use theme/token-driven defaults (and/or size-aware logic) instead of a fixed 30. [api mismatch]

Severity Level: Major ⚠️
- ⚠️ Add Dataset footer buttons render at mismatched heights.
- ⚠️ SQL Lab run-dropdown button ignores AntD size contract.
- ⚠️ Theme buttonControlHeight tokens not honored by dropdown.
Steps of Reproduction ✅
1. Open `superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx` and
note the default button height logic at lines 137-145, where `defaultHeight` is computed
as `theme.buttonControlHeight ?? 32` for normal buttons and `theme.buttonControlHeightSM
?? 30` only when `buttonSize === 'small'`.

2. Open
`superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx` and
observe that at line 61 the wrapper forces `.ant-btn { height:
${styleConfig?.controlHeight ?? 30}px; }`, ignoring any AntD `size` prop and the shared
`buttonControlHeight*` tokens.

3. Open `superset-frontend/src/features/datasets/AddDataset/Footer/index.tsx` and see the
footer layout at lines 144-148 where a `Button` (Cancel) and a `DropdownButton` (Create
dataset) are rendered side by side with no `styleConfig` overrides passed to
`DropdownButton`.

4. Run the UI and navigate to the "Add dataset" flow so that `Footer` renders; the
`Button` uses the 32px default from `Button/index.tsx` while `DropdownButton` uses the
hardcoded 30px height from `DropdownButton/index.tsx:61`, causing visible vertical
misalignment between the two buttons and, for any future caller that sets `size="small" |
"large"` on `DropdownButton` (supported via `DropdownButtonProps` in `types.ts:31`), the
forced 30px height will continue to ignore that `size` prop.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx
**Line:** 61:61
**Comment:**
	*Api Mismatch: This hardcoded fallback height ignores the component's `size` prop and also diverges from the shared button token defaults, so default dropdown buttons render at 30px while regular default buttons render at 32px. That creates visible misalignment in mixed button rows (for example, the dataset footer) and breaks the expected AntD size contract. Use theme/token-driven defaults (and/or size-aware logic) instead of a fixed `30`.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

box-shadow: ${styleConfig?.boxShadow ?? 'none'};
font-size: ${styleConfig?.fontSize ?? theme.fontSizeSM}px;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: The fallback font size is wired to fontSizeSM instead of the shared button font token, so global button typography customization does not propagate to DropdownButton and this component drifts from Button styling. Align this fallback with the same button token path used by the main button component. [logic error]

Severity Level: Major ⚠️
- ⚠️ DropdownButton text ignores theme.buttonFontSize overrides.
- ⚠️ Adjacent Button and DropdownButton show mismatched typography.
- ⚠️ Theming API for button fonts applied inconsistently across components.
Steps of Reproduction ✅
1. Open `superset-frontend/packages/superset-core/src/theme/types.ts` and note that
`SupersetTheme` exposes a `buttonFontSize?: number` token at lines 244-251 as part of the
public theming surface.

2. Open `superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx` and
see at line 122 that `btnFontSize` is computed as `theme.buttonFontSize ??
theme.fontSizeSM`, and at line 153 this `btnFontSize` is applied to
`resolvedStyleConfig.fontSize`, which is then used as `fontSize` in the button's `css`
block at line 211—so any theme overriding `buttonFontSize` changes all core `Button` text
sizes.

3. Open
`superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx` and
observe at line 63 that the dropdown button text uses `font-size: ${styleConfig?.fontSize
?? theme.fontSizeSM}px;`, bypassing `theme.buttonFontSize` entirely when `styleConfig` is
not provided.

4. In a deployment that sets `buttonFontSize` to a value different from `fontSizeSM` via
the Superset theme (as allowed by `SupersetTheme`), render any `DropdownButton` usage such
as the Add Dataset footer (`src/features/datasets/AddDataset/Footer/index.tsx:144-148`) or
the SQL Lab run menu (`src/SqlLab/components/RunQueryActionButton/index.tsx` when
`overlayCreateAsMenu` is truthy); the standard `Button` text will reflect the customized
`buttonFontSize` while `DropdownButton` text remains at `fontSizeSM`, causing inconsistent
typography between adjacent buttons.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/packages/superset-ui-core/src/components/DropdownButton/index.tsx
**Line:** 63:63
**Comment:**
	*Logic Error: The fallback font size is wired to `fontSizeSM` instead of the shared button font token, so global button typography customization does not propagate to `DropdownButton` and this component drifts from `Button` styling. Align this fallback with the same button token path used by the main button component.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

font-weight: ${styleConfig?.fontWeight ?? theme.fontWeightStrong};
}
`,
]}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,15 @@ import { type ComponentProps } from 'react';
import { Dropdown } from 'antd';
import type { TooltipPlacement } from '../Tooltip/types';

export type DropdownButtonStyleConfig = {
controlHeight?: number;
fontSize?: number;
fontWeight?: number;
boxShadow?: string;
};

export type DropdownButtonProps = ComponentProps<typeof Dropdown.Button> & {
tooltip?: string;
tooltipPlacement?: TooltipPlacement;
styleConfig?: DropdownButtonStyleConfig;
};
Original file line number Diff line number Diff line change
Expand Up @@ -46,27 +46,30 @@ export function Label(props: LabelProps) {
const borderColorHover = onClick ? baseColor.borderHover : borderColor;

const labelStyles = css`
transition: background-color ${theme.motionDurationMid};
white-space: nowrap;
cursor: ${onClick ? 'pointer' : 'default'};
overflow: hidden;
text-overflow: ellipsis;
background-color: ${backgroundColor};
border-radius: 8px;
border-color: ${borderColor};
padding: 0.35em 0.8em;
line-height: 1;
color: ${color};
display: inline-flex;
vertical-align: middle;
align-items: center;
max-width: 100%;
&:hover {
background-color: ${backgroundColorHover};
border-color: ${borderColorHover};
opacity: 1;
// NOTE: && bumps specificity to beat antd Tag defaults
&& {
transition: background-color ${theme.motionDurationMid};
white-space: nowrap;
cursor: ${onClick ? 'pointer' : 'default'};
overflow: hidden;
text-overflow: ellipsis;
background-color: ${backgroundColor};
border-radius: ${theme.labelBorderRadius ?? 8}px;
border-color: ${borderColor};
padding: 0.35em 0.8em;
line-height: 1;
color: ${color};
display: inline-flex;
vertical-align: middle;
align-items: center;
max-width: 100%;
&:hover {
background-color: ${backgroundColorHover};
border-color: ${borderColorHover};
opacity: 1;
}
${monospace ? `font-family: ${theme.fontFamilyCode};` : ''}
}
${monospace ? `font-family: ${theme.fontFamilyCode};` : ''}
`;

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,11 @@ import { CertifiedBadge } from '../CertifiedBadge';
import { Button } from '../Button';

export const menuTriggerStyles = (theme: SupersetTheme) => css`
width: ${theme.sizeUnit * 8}px;
height: ${theme.sizeUnit * 8}px;
width: ${theme.buttonControlHeight ?? theme.sizeUnit * 8}px;
height: ${theme.buttonControlHeight ?? theme.sizeUnit * 8}px;
padding: 0;
border: 1px solid ${theme.colorPrimary};
border-radius: ${theme.buttonBorderRadius ?? theme.borderRadius}px;

&.ant-btn > span.anticon {
line-height: 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,11 @@ export const StyledContainer = styled.div<{ headerPosition: string }>`
export const StyledSelect = styled(Select, {
shouldForwardProp: prop => prop !== 'headerPosition' && prop !== 'oneLine',
})<{ headerPosition?: string; oneLine?: boolean }>`
${({ theme, headerPosition, oneLine }) => `
${({ theme, headerPosition, oneLine }) => {
const useSubtleOptionHover = theme.selectOptionActiveOutline === false;
return `
.ant-select-item-option-active:not(.ant-select-item-option-disabled) {
outline: 2px solid ${theme.colorPrimary};
outline-offset: -2px;
${useSubtleOptionHover ? 'outline: none;' : `outline: 2px solid ${theme.colorPrimary}; outline-offset: -2px;`}
}
flex: ${headerPosition === 'left' ? 1 : 0};
line-height: ${theme.sizeXL}px;
Expand Down Expand Up @@ -82,7 +83,8 @@ export const StyledSelect = styled(Select, {
}
`
};
`}
`;
}}
`;

export const NoElement = styled.span`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const CardContainer = styled.div<{ showThumbnails?: boolean }>`
display: grid;
justify-content: start;
grid-gap: ${theme.sizeUnit * 12}px ${theme.sizeUnit * 4}px;
grid-template-columns: repeat(auto-fit, 300px);
grid-template-columns: repeat(auto-fit, ${theme.sizeUnit * 75}px);
margin-top: ${theme.sizeUnit * -6}px;
padding: ${
showThumbnails
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,9 @@ const StyledDashboardContent = styled.div<{
padding: ${theme.sizeUnit * 4}px;
box-sizing: border-box;
overflow-y: visible;
border: 1px solid ${theme.colorBorder};
border-radius: ${theme.borderRadius}px;
box-shadow: ${theme.boxShadowTertiary};

// transitionable traits to show filter relevance
transition:
Expand All @@ -337,7 +340,7 @@ const StyledDashboardContent = styled.div<{

&.fade-out {
border-radius: ${theme.borderRadius}px;
box-shadow: 0 0 0 1px ${addAlpha(theme.colorBorder, 0.5)};
box-shadow: ${theme.boxShadowTertiary};
}

& .missing-chart-container {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ const StyledTabsContainer = styled.div<{ isDragging?: boolean }>`
width: 100%;
background-color: ${({ theme }) => theme.colorBgContainer};

.ant-tabs > .ant-tabs-nav {
margin-bottom: ${({ theme }) => theme.sizeUnit * 4}px;
}

& .dashboard-component-tabs-content {
height: 100%;
}
Expand Down
Loading
Loading