codeant-ai-for-open-source[bot] commented on code in PR #38604:
URL: https://github.com/apache/superset/pull/38604#discussion_r2959660401
##########
superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx:
##########
@@ -132,12 +193,10 @@ export function Button(props: ButtonProps) {
'& > span > :first-of-type': {
marginRight: firstChildMargin,
},
- ':not(:hover)': effectiveButtonStyle === 'secondary' &&
- !disabled && {
- // NOTE: This is the best we can do contrast wise for the
secondary button using antd tokens
- // and abusing the semantics. Should be revisited when possible.
https://github.com/apache/superset/pull/34253#issuecomment-3104834692
- color: `${theme.colorPrimaryTextHover} !important`,
- },
+ // Secondary button hover/active states via CSS
+ ...(effectiveButtonStyle === 'secondary' &&
+ !disabled &&
+ getSecondaryButtonHoverStyles(theme)),
}}
icon={icon}
{...restProps}
Review Comment:
**Suggestion:** The new secondary theming style is applied before
`{...restProps}`, so any caller-provided `style` prop in `restProps` overwrites
it entirely. This breaks the intended secondary theme tokens whenever consumers
pass unrelated inline styles (for example margin), because the secondary
color/background/border styles are dropped. Apply `restProps` first and then
set a merged `style` object so secondary theme styles are preserved while still
allowing explicit caller overrides. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Secondary token styling lost when inline style is passed.
- ⚠️ Button Storybook gallery shows incorrect secondary theming behavior.
- ⚠️ External consumers may unintentionally disable secondary theme tokens.
```
</details>
```suggestion
{...restProps}
style={
effectiveButtonStyle === 'secondary' && !disabled
? { ...getSecondaryButtonStyle(theme), ...(restProps.style ?? {}) }
: restProps.style
}
css={{
display: 'inline-flex',
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,
marginLeft: 0,
'& + .superset-button:not(.ant-btn-compact-item)': {
marginLeft: theme.sizeUnit * 2,
},
'& > span > :first-of-type': {
marginRight: firstChildMargin,
},
// Secondary button hover/active states via CSS
...(effectiveButtonStyle === 'secondary' &&
!disabled &&
getSecondaryButtonHoverStyles(theme)),
}}
icon={icon}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open `ButtonGallery` in
`superset-frontend/packages/superset-ui-core/src/components/Button/Button.stories.tsx:73-92`;
it iterates styles including `'secondary'` (`:33-39`) and passes `style={{
marginRight:
20, marginBottom: 10 }}` to every `<Button>` (`:79-85`).
2. This calls `Button` in
`superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx`;
`style` is
part of `ButtonProps` via `Omit<AntdButtonProps, 'css'>` in `types.ts:44`,
and is captured
inside `...restProps` (`index.tsx:103-117`).
3. In the render path, secondary theme inline styles are set with
`style={secondaryStyle}`
(`index.tsx` PR hunk line 177), but then `{...restProps}` is applied later
(`line 202`),
so `restProps.style` replaces the whole `style` prop.
4. Result: `getSecondaryButtonStyle()` output (`index.tsx:63-67`) is dropped
whenever
caller supplies any inline style object; secondary token-based default
colors/border are
not applied.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Button/index.tsx
**Line:** 177:202
**Comment:**
*Logic Error: The new secondary theming style is applied before
`{...restProps}`, so any caller-provided `style` prop in `restProps` overwrites
it entirely. This breaks the intended secondary theme tokens whenever consumers
pass unrelated inline styles (for example margin), because the secondary
color/background/border styles are dropped. Apply `restProps` first and then
set a merged `style` object so secondary theme styles are preserved while still
allowing explicit caller overrides.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38604&comment_hash=348c6de66edbfa35eb8e022243454b88bfe3cbea69979d2a607772a627af61f7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38604&comment_hash=348c6de66edbfa35eb8e022243454b88bfe3cbea69979d2a607772a627af61f7&reaction=dislike'>👎</a>
--
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]
