korbit-ai[bot] commented on code in PR #31575:
URL: https://github.com/apache/superset/pull/31575#discussion_r1894535611
##########
superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-style/Theme.stories.tsx:
##########
@@ -25,23 +25,96 @@ export default {
export const ThemeColors = () => {
const { colors } = supersetTheme;
- return Object.keys(colors).map(collection => (
+
+ // Define tones to be displayed in columns
+ const tones = [
+ 'dark2',
+ 'dark1',
+ 'base',
+ 'light1',
+ 'light2',
+ 'light3',
+ 'light4',
+ 'light5',
+ ];
+ const colorTypes = [
+ 'primary',
+ 'secondary',
+ 'grayscale',
+ 'error',
+ 'warning',
+ 'alert',
+ 'success',
+ 'info',
+ ];
+ return (
<div>
- <h2>{collection}</h2>
- <table style={{ width: '300px' }}>
- {Object.keys(colors[collection]).map(k => {
- const hex = colors[collection][k];
- return (
- <tr>
- <td>{k}</td>
- <td>
- <code>{hex}</code>
+ <h1>Theme Colors</h1>
+ <table
+ style={{ borderCollapse: 'collapse', width: '100%', textAlign: 'left'
}}
+ >
+ <thead>
+ <tr>
+ <th style={{ border: '1px solid #ddd', padding: '8px' }}>
+ Category
+ </th>
+ {tones.map(tone => (
+ <th
+ key={tone}
+ style={{ border: '1px solid #ddd', padding: '8px' }}
+ >
+ {tone}
+ </th>
+ ))}
+ </tr>
+ </thead>
+ <tbody>
+ {colorTypes.map(category => (
+ <tr key={category}>
+ <td style={{ border: '1px solid #ddd', padding: '8px' }}>
+ <strong>{category}</strong>
</td>
- <td style={{ width: '150px', backgroundColor: hex }} />
+ {tones.map(tone => {
+ const color = colors[category][tone];
+ return (
+ <td
+ key={tone}
+ style={{
+ border: '1px solid #ddd',
+ padding: '8px',
+ backgroundColor: color || '#fff',
+ }}
+ >
+ {color ? <code>{color}</code> : '-'}
+ </td>
+ );
+ })}
</tr>
- );
- })}
+ ))}
+ </tbody>
</table>
+ <h3>
+ text.label: <code>{colors.text.label}</code>
+ </h3>
+ <div style={{ color: `#${colors.text.label}` }}>
Review Comment:
### Invalid Color Format in Text Label Styling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code incorrectly prepends '#' to the color value from colors.text.label,
which likely already includes the '#' prefix, resulting in an invalid color
format.
###### Why this matters
This will cause the text to be rendered with an invalid color value, making
it invisible or using the browser's default color instead of the intended theme
color.
###### Suggested change
Remove the '#' prefix: style={{ color: colors.text.label }}
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:881a7e23-e449-4398-9f45-c806fb13ea02 -->
##########
superset-frontend/src/components/CachedLabel/index.tsx:
##########
@@ -34,7 +35,7 @@ const CacheLabel: FC<CacheLabelProps> = ({
onClick,
cachedTimestamp,
}) => {
- const [hovered, setHovered] = useState(false);
+ const [hovered] = useState(false);
const labelType = hovered ? 'primary' : 'default';
Review Comment:
### Non-functional hover state <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The hovered state is maintained but the mouse events (onMouseOver and
onMouseOut) that would update it have been removed, making the hover effect
non-functional.
###### Why this matters
The label will always remain in the 'default' state since the hovered state
can never change to true, breaking the intended hover interaction functionality.
###### Suggested change
Either remove the unused hover state logic completely:
const labelType = 'default';
Or restore the mouse event handlers:
const [hovered, setHovered] = useState(false);
// ... in Label component:
onMouseOver={() => setHovered(true)}
onMouseOut={() => setHovered(false)}
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:b94a5315-f709-409b-9029-a604f4d33e13 -->
##########
superset-frontend/src/components/Icons/Icon.tsx:
##########
@@ -36,6 +36,10 @@ const AntdIconComponent = ({
export const StyledIcon = styled(AntdIconComponent)<IconType>`
${({ iconColor }) => iconColor && `color: ${iconColor};`};
+ span {
+ // Hack to fix issues with some icons
+ line-height: 0px !important;
Review Comment:
### Brittle icon fix using CSS hack <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using !important and a hack to fix icon issues could lead to unintended side
effects with different icon sets or future updates.
###### Why this matters
This brittle solution might break when new icons are added or when the
underlying icon library is updated, potentially causing visual inconsistencies.
###### Suggested change
Investigate the root cause of the icon alignment issues and implement a more
robust solution, possibly by properly configuring the icon component's baseline
alignment or using proper spacing properties.
</details>
###### Chat with Korbit by mentioning @korbit-ai, and give a 👍 or 👎 to help
Korbit improve your reviews.
<!--- korbi internal id:e77f722a-a326-4119-8edf-738d8fadd891 -->
--
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]
