rusackas commented on code in PR #35506:
URL: https://github.com/apache/superset/pull/35506#discussion_r2461989614
##########
superset-frontend/packages/superset-ui-demo/storybook/stories/superset-ui-theme/Theme.stories.tsx:
##########
@@ -66,15 +59,17 @@ const AntDFunctionalColors = () => {
<strong>{type}</strong>
</td>
{variants.map(variant => {
- const color = themeObject.getColorVariants(type)[variant];
+ // Map to actual theme token names
+ const tokenName = `color${type.charAt(0).toUpperCase() +
type.slice(1)}${variant.charAt(0).toUpperCase() + variant.slice(1)}`;
+ const color = (supersetTheme as any)[tokenName];
return (
<td
key={variant}
style={{
border: '1px solid #ddd',
padding: '8px',
backgroundColor: color || 'transparent',
- color: `color${type}${variant}`,
+ color: color === 'transparent' ? 'black' : undefined,
Review Comment:
1. Low impact: This appears to be in a styling context where transparent
backgrounds are likely intentional design choices, not accidental bugs
2. Working as intended: The current logic probably works fine in practice
- falsy colors defaulting to transparent background is a reasonable fallback
3. Risk vs benefit: Changing color handling logic could have unintended
visual effects across the application that would be hard to test comprehensively
4. Edge case: The scenario where this inconsistency causes actual problems
(invisible text on transparent background) is likely rare and would be caught
in normal usage/testing
--
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]
