Skip to content

Commit 82a74c8

Browse files
alexandrusoarealexandrusoare
authored
fix(button): Theming configurations for button elements is not consistent (#38604)
1 parent 6b9dd23 commit 82a74c8

File tree

4 files changed

+299
-6
lines changed

4 files changed

+299
-6
lines changed

‎superset-frontend/packages/superset-core/src/theme/types.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,55 @@ export interface SupersetSpecificTokens {
168168
* Defaults to colorPrimaryBgHover if not specified.
169169
*/
170170
colorEditorSelection?: string;
171+
172+
// Secondary button tokens (Superset-specific)
173+
// Ant Design's filled variant has no component tokens, so we provide our own.
174+
// These fallback to colorPrimary* derived tokens when not set.
175+
/**
176+
* Text color for secondary buttons.
177+
* Fallback: colorPrimary
178+
*/
179+
buttonSecondaryColor?: string;
180+
/**
181+
* Background color for secondary buttons.
182+
* Fallback: colorPrimaryBg
183+
*/
184+
buttonSecondaryBg?: string;
185+
/**
186+
* Border color for secondary buttons.
187+
* Fallback: transparent
188+
*/
189+
buttonSecondaryBorderColor?: string;
190+
/**
191+
* Text color for secondary buttons on hover.
192+
* Fallback: colorPrimary
193+
*/
194+
buttonSecondaryHoverColor?: string;
195+
/**
196+
* Background color for secondary buttons on hover.
197+
* Fallback: colorPrimaryBgHover
198+
*/
199+
buttonSecondaryHoverBg?: string;
200+
/**
201+
* Border color for secondary buttons on hover.
202+
* Fallback: transparent
203+
*/
204+
buttonSecondaryHoverBorderColor?: string;
205+
/**
206+
* Text color for secondary buttons when active/pressed.
207+
* Fallback: colorPrimary
208+
*/
209+
buttonSecondaryActiveColor?: string;
210+
/**
211+
* Background color for secondary buttons when active/pressed.
212+
* Fallback: colorPrimaryBorder
213+
*/
214+
buttonSecondaryActiveBg?: string;
215+
/**
216+
* Border color for secondary buttons when active/pressed.
217+
* Fallback: transparent
218+
*/
219+
buttonSecondaryActiveBorderColor?: string;
171220
}
172221

173222
/**

‎superset-frontend/packages/superset-ui-core/src/components/Button/Button.test.tsx‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ import {
2323
SIZES as buttonSizes,
2424
STYLES as buttonStyles,
2525
} from './Button.stories';
26+
import {
27+
getSecondaryButtonStyle,
28+
getSecondaryButtonHoverStyles,
29+
} from './index';
30+
import type { SupersetTheme } from '@apache-superset/core/theme';
2631

2732
test('works with an onClick handler', () => {
2833
const mockAction = jest.fn();
@@ -47,3 +52,165 @@ test('All the sorybook gallery variants mount', () => {
4752

4853
expect(getAllByRole('button')).toHaveLength(permutationCount);
4954
});
55+
56+
test('secondary button renders without errors', () => {
57+
const { getByRole } = render(
58+
<Button buttonStyle="secondary">Secondary</Button>,
59+
);
60+
expect(getByRole('button')).toBeInTheDocument();
61+
});
62+
63+
test('getSecondaryButtonStyle uses fallback tokens when custom tokens not set', () => {
64+
const mockTheme = {
65+
colorPrimary: '#2893B3',
66+
colorPrimaryBg: '#e6f4f7',
67+
colorPrimaryBgHover: '#cce9ef',
68+
colorPrimaryBorder: '#99d3df',
69+
} as SupersetTheme;
70+
71+
const styles = getSecondaryButtonStyle(mockTheme);
72+
73+
// Default state uses inline styles (no !important needed)
74+
expect(styles.color).toBe('#2893B3');
75+
expect(styles.backgroundColor).toBe('#e6f4f7');
76+
expect(styles.borderColor).toBe('transparent');
77+
});
78+
79+
test('getSecondaryButtonHoverStyles uses fallback tokens when custom tokens not set', () => {
80+
const mockTheme = {
81+
colorPrimary: '#2893B3',
82+
colorPrimaryBg: '#e6f4f7',
83+
colorPrimaryBgHover: '#cce9ef',
84+
colorPrimaryBorder: '#99d3df',
85+
} as SupersetTheme;
86+
87+
const hoverStyles = getSecondaryButtonHoverStyles(mockTheme);
88+
89+
// Hover/active states use CSS with !important for specificity
90+
expect(hoverStyles['&:hover'].backgroundColor).toBe('#cce9ef !important');
91+
expect(hoverStyles['&:active'].backgroundColor).toBe('#99d3df !important');
92+
});
93+
94+
test('getSecondaryButtonStyle uses custom tokens when provided', () => {
95+
const mockTheme = {
96+
colorPrimary: '#2893B3',
97+
colorPrimaryBg: '#e6f4f7',
98+
colorPrimaryBgHover: '#cce9ef',
99+
colorPrimaryBorder: '#99d3df',
100+
// Custom secondary button tokens
101+
buttonSecondaryColor: '#custom-color',
102+
buttonSecondaryBg: '#custom-bg',
103+
buttonSecondaryBorderColor: '#custom-border',
104+
} as SupersetTheme;
105+
106+
const styles = getSecondaryButtonStyle(mockTheme);
107+
108+
// Default state uses inline styles (no !important needed)
109+
expect(styles.color).toBe('#custom-color');
110+
expect(styles.backgroundColor).toBe('#custom-bg');
111+
expect(styles.borderColor).toBe('#custom-border');
112+
});
113+
114+
test('getSecondaryButtonHoverStyles uses custom tokens when provided', () => {
115+
const mockTheme = {
116+
colorPrimary: '#2893B3',
117+
colorPrimaryBg: '#e6f4f7',
118+
colorPrimaryBgHover: '#cce9ef',
119+
colorPrimaryBorder: '#99d3df',
120+
// Custom secondary button tokens
121+
buttonSecondaryHoverColor: '#custom-hover-color',
122+
buttonSecondaryHoverBg: '#custom-hover-bg',
123+
buttonSecondaryHoverBorderColor: '#custom-hover-border',
124+
buttonSecondaryActiveColor: '#custom-active-color',
125+
buttonSecondaryActiveBg: '#custom-active-bg',
126+
buttonSecondaryActiveBorderColor: '#custom-active-border',
127+
} as SupersetTheme;
128+
129+
const hoverStyles = getSecondaryButtonHoverStyles(mockTheme);
130+
131+
// Hover/active states use CSS with !important for specificity
132+
expect(hoverStyles['&:hover'].color).toBe('#custom-hover-color !important');
133+
expect(hoverStyles['&:hover'].backgroundColor).toBe(
134+
'#custom-hover-bg !important',
135+
);
136+
expect(hoverStyles['&:hover'].borderColor).toBe(
137+
'#custom-hover-border !important',
138+
);
139+
expect(hoverStyles['&:active'].color).toBe('#custom-active-color !important');
140+
expect(hoverStyles['&:active'].backgroundColor).toBe(
141+
'#custom-active-bg !important',
142+
);
143+
expect(hoverStyles['&:active'].borderColor).toBe(
144+
'#custom-active-border !important',
145+
);
146+
});
147+
148+
test('getSecondaryButtonStyle supports partial token overrides', () => {
149+
const mockTheme = {
150+
colorPrimary: '#2893B3',
151+
colorPrimaryBg: '#e6f4f7',
152+
colorPrimaryBgHover: '#cce9ef',
153+
colorPrimaryBorder: '#99d3df',
154+
// Only override some tokens
155+
buttonSecondaryBg: '#custom-bg',
156+
buttonSecondaryBorderColor: '#custom-border',
157+
} as SupersetTheme;
158+
159+
const styles = getSecondaryButtonStyle(mockTheme);
160+
161+
// Should use custom values where provided (no !important for inline styles)
162+
expect(styles.backgroundColor).toBe('#custom-bg');
163+
expect(styles.borderColor).toBe('#custom-border');
164+
// Should fallback to Ant Design tokens where not provided
165+
expect(styles.color).toBe('#2893B3');
166+
});
167+
168+
test('getSecondaryButtonHoverStyles supports partial token overrides', () => {
169+
const mockTheme = {
170+
colorPrimary: '#2893B3',
171+
colorPrimaryBg: '#e6f4f7',
172+
colorPrimaryBgHover: '#cce9ef',
173+
colorPrimaryBorder: '#99d3df',
174+
// Only override hover bg
175+
buttonSecondaryHoverBg: '#custom-hover-bg',
176+
} as SupersetTheme;
177+
178+
const hoverStyles = getSecondaryButtonHoverStyles(mockTheme);
179+
180+
// Should use custom value where provided
181+
expect(hoverStyles['&:hover'].backgroundColor).toBe(
182+
'#custom-hover-bg !important',
183+
);
184+
// Should fallback to Ant Design tokens where not provided
185+
expect(hoverStyles['&:hover'].color).toBe('#2893B3 !important');
186+
expect(hoverStyles['&:active'].backgroundColor).toBe('#99d3df !important');
187+
});
188+
189+
test('getSecondaryButtonStyle falls back when tokens are empty strings', () => {
190+
const mockTheme = {
191+
colorPrimary: '#2893B3',
192+
colorPrimaryBg: '#e6f4f7',
193+
buttonSecondaryColor: '',
194+
buttonSecondaryBg: '',
195+
buttonSecondaryBorderColor: '',
196+
} as SupersetTheme;
197+
198+
const styles = getSecondaryButtonStyle(mockTheme);
199+
200+
// Empty strings should trigger fallback to primary tokens
201+
expect(styles.color).toBe('#2893B3');
202+
expect(styles.backgroundColor).toBe('#e6f4f7');
203+
expect(styles.borderColor).toBe('transparent');
204+
});
205+
206+
test('secondary button merges consumer style with theme styles', () => {
207+
const { getByRole } = render(
208+
<Button buttonStyle="secondary" style={{ marginTop: 10, padding: 20 }}>
209+
Styled Secondary
210+
</Button>,
211+
);
212+
const button = getByRole('button');
213+
214+
// Consumer styles should be applied
215+
expect(button).toHaveStyle({ marginTop: '10px', padding: '20px' });
216+
});

‎superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx‎

Lines changed: 69 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import cx from 'classnames';
2121
import { Button as AntdButton } from 'antd';
2222
import { useTheme } from '@apache-superset/core/theme';
2323
import { Tooltip } from '../Tooltip';
24+
import type { SupersetTheme } from '@apache-superset/core/theme';
2425
import type {
2526
ButtonColorType,
2627
ButtonProps,
@@ -30,6 +31,59 @@ import type {
3031
OnClickHandler,
3132
} from './types';
3233

34+
/**
35+
* Secondary Button Theming
36+
*
37+
* Ant Design's "filled" variant (used for secondary buttons) has no component-level
38+
* tokens for customization. To enable full theming of secondary buttons, we use
39+
* Superset-specific tokens (buttonSecondary*) with fallbacks to Ant Design's
40+
* colorPrimary* derived tokens.
41+
*
42+
* Implementation approach (follows PR #38679 pattern for label tokens):
43+
* - Default state: Applied via inline `style` prop (higher specificity than CSS classes)
44+
* - Hover/Active states: Applied via `css` prop with !important (pseudo-selectors
45+
* cannot be applied via inline styles)
46+
*
47+
* Available tokens (all optional, with sensible fallbacks):
48+
* - buttonSecondaryColor: Text color (fallback: colorPrimary)
49+
* - buttonSecondaryBg: Background color (fallback: colorPrimaryBg)
50+
* - buttonSecondaryBorderColor: Border color (fallback: transparent)
51+
* - buttonSecondaryHoverColor: Hover text color (fallback: colorPrimary)
52+
* - buttonSecondaryHoverBg: Hover background (fallback: colorPrimaryBgHover)
53+
* - buttonSecondaryHoverBorderColor: Hover border (fallback: transparent)
54+
* - buttonSecondaryActiveColor: Active/pressed text color (fallback: colorPrimary)
55+
* - buttonSecondaryActiveBg: Active/pressed background (fallback: colorPrimaryBorder)
56+
* - buttonSecondaryActiveBorderColor: Active/pressed border (fallback: transparent)
57+
*/
58+
59+
/**
60+
* Generates inline styles for secondary buttons (default state).
61+
* Inline styles have higher specificity than CSS classes, so no !important needed.
62+
*/
63+
export const getSecondaryButtonStyle = (theme: SupersetTheme) => ({
64+
color: theme.buttonSecondaryColor || theme.colorPrimary,
65+
backgroundColor: theme.buttonSecondaryBg || theme.colorPrimaryBg,
66+
borderColor: theme.buttonSecondaryBorderColor || 'transparent',
67+
});
68+
69+
/**
70+
* Generates CSS styles for secondary button hover/active states.
71+
* Must use CSS (not inline styles) since pseudo-selectors cannot be applied via style prop.
72+
* Uses !important to override Ant Design's default styles.
73+
*/
74+
export const getSecondaryButtonHoverStyles = (theme: SupersetTheme) => ({
75+
'&:hover': {
76+
color: `${theme.buttonSecondaryHoverColor || theme.colorPrimary} !important`,
77+
backgroundColor: `${theme.buttonSecondaryHoverBg || theme.colorPrimaryBgHover} !important`,
78+
borderColor: `${theme.buttonSecondaryHoverBorderColor || 'transparent'} !important`,
79+
},
80+
'&:active': {
81+
color: `${theme.buttonSecondaryActiveColor || theme.colorPrimary} !important`,
82+
backgroundColor: `${theme.buttonSecondaryActiveBg || theme.colorPrimaryBorder} !important`,
83+
borderColor: `${theme.buttonSecondaryActiveBorderColor || 'transparent'} !important`,
84+
},
85+
});
86+
3387
const BUTTON_STYLE_MAP: Record<
3488
ButtonStyle,
3589
{
@@ -98,6 +152,12 @@ export function Button(props: ButtonProps) {
98152

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

155+
// Secondary button inline styles (default state) - inline styles override CSS classes
156+
const secondaryStyle =
157+
effectiveButtonStyle === 'secondary' && !disabled
158+
? getSecondaryButtonStyle(theme)
159+
: undefined;
160+
101161
const button = (
102162
<AntdButton
103163
href={disabled ? undefined : href}
@@ -132,15 +192,18 @@ export function Button(props: ButtonProps) {
132192
'& > span > :first-of-type': {
133193
marginRight: firstChildMargin,
134194
},
135-
':not(:hover)': effectiveButtonStyle === 'secondary' &&
136-
!disabled && {
137-
// NOTE: This is the best we can do contrast wise for the secondary button using antd tokens
138-
// and abusing the semantics. Should be revisited when possible. https://github.com/apache/superset/pull/34253#issuecomment-3104834692
139-
color: `${theme.colorPrimaryTextHover} !important`,
140-
},
195+
// Secondary button hover/active states via CSS
196+
...(effectiveButtonStyle === 'secondary' &&
197+
!disabled &&
198+
getSecondaryButtonHoverStyles(theme)),
141199
}}
142200
icon={icon}
143201
{...restProps}
202+
style={
203+
secondaryStyle
204+
? { ...secondaryStyle, ...restProps.style }
205+
: restProps.style
206+
}
144207
>
145208
{children}
146209
</AntdButton>

‎superset-frontend/src/theme/utils/antdTokenNames.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,20 @@ const SUPERSET_CUSTOM_TOKENS: Set<string> = new Set([
4747

4848
// Font loading
4949
'fontUrls',
50+
51+
// Editor tokens
52+
'colorEditorSelection',
53+
54+
// Secondary button tokens
55+
'buttonSecondaryColor',
56+
'buttonSecondaryBg',
57+
'buttonSecondaryBorderColor',
58+
'buttonSecondaryHoverColor',
59+
'buttonSecondaryHoverBg',
60+
'buttonSecondaryHoverBorderColor',
61+
'buttonSecondaryActiveColor',
62+
'buttonSecondaryActiveBg',
63+
'buttonSecondaryActiveBorderColor',
5064
]);
5165

5266
/**

0 commit comments

Comments
 (0)