korbit-ai[bot] commented on code in PR #35211:
URL: https://github.com/apache/superset/pull/35211#discussion_r2362486158
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -169,12 +170,24 @@ function cellOffset({
function cellBackground({
value,
colorPositiveNegative = false,
+ theme,
}: {
value: number;
colorPositiveNegative: boolean;
+ theme: SupersetTheme;
}) {
- const r = colorPositiveNegative && value < 0 ? 150 : 0;
- return `rgba(${r},0,0,0.2)`;
+ if (!colorPositiveNegative) {
+ const color = isThemeDark(theme)
+ ? 'rgba(255,255,255,0.3)'
+ : 'rgba(0,0,0,0.2)';
+ return color;
+ }
+
+ const r = value < 0 ? 150 : 0;
+ const color = isThemeDark(theme)
+ ? `rgba(255,255,255,0.3)`
+ : `rgba(${r},0,0,0.2)`;
+ return color;
Review Comment:
### Unnecessary computation in dark theme path <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The cellBackground function computes the red color component 'r' but only
uses it in the light theme case, creating unnecessary computation for dark
themes.
###### Why this matters
When using dark theme, the red component calculation is wasted since it's
not used in the final color string, leading to inefficient resource usage.
###### Suggested change ∙ *Feature Preview*
Move the red component calculation inside the light theme condition:
```typescript
const color = isThemeDark(theme)
? 'rgba(255,255,255,0.3)'
: `rgba(${value < 0 ? 150 : 0},0,0,0.2)`;
return color;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32fdc652-ddf8-4d5e-9a75-50f95d127ea5/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32fdc652-ddf8-4d5e-9a75-50f95d127ea5?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32fdc652-ddf8-4d5e-9a75-50f95d127ea5?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32fdc652-ddf8-4d5e-9a75-50f95d127ea5?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/32fdc652-ddf8-4d5e-9a75-50f95d127ea5)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:156de76b-9170-4563-bf19-8e690e173992 -->
[](156de76b-9170-4563-bf19-8e690e173992)
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -169,12 +170,24 @@ function cellOffset({
function cellBackground({
value,
colorPositiveNegative = false,
+ theme,
}: {
value: number;
colorPositiveNegative: boolean;
+ theme: SupersetTheme;
}) {
- const r = colorPositiveNegative && value < 0 ? 150 : 0;
- return `rgba(${r},0,0,0.2)`;
+ if (!colorPositiveNegative) {
+ const color = isThemeDark(theme)
+ ? 'rgba(255,255,255,0.3)'
+ : 'rgba(0,0,0,0.2)';
+ return color;
+ }
+
+ const r = value < 0 ? 150 : 0;
+ const color = isThemeDark(theme)
+ ? `rgba(255,255,255,0.3)`
+ : `rgba(${r},0,0,0.2)`;
Review Comment:
### Dark theme loses positive/negative color distinction <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
When colorPositiveNegative is true, the function ignores the
positive/negative color distinction in dark theme and always returns white with
opacity, losing the red color for negative values.
###### Why this matters
This breaks the visual distinction between positive and negative values in
dark mode, making it impossible for users to quickly identify negative values
through color coding.
###### Suggested change ∙ *Feature Preview*
Modify the logic to preserve the red color for negative values in dark theme:
```typescript
const r = value < 0 ? 150 : 0;
const color = isThemeDark(theme)
? `rgba(${255 - r},${255 - r},255,0.3)` // Maintains red tint for negatives
: `rgba(${r},0,0,0.2)`;
```
This ensures negative values show as a purple/pink tint (mixing red with
white) in dark mode while positive values remain white.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11dce370-63e9-45e5-82f9-3a44fe28a3a1/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11dce370-63e9-45e5-82f9-3a44fe28a3a1?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11dce370-63e9-45e5-82f9-3a44fe28a3a1?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11dce370-63e9-45e5-82f9-3a44fe28a3a1?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/11dce370-63e9-45e5-82f9-3a44fe28a3a1)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6910c44b-edaf-4104-b703-2a6ddd3e09ed -->
[](6910c44b-edaf-4104-b703-2a6ddd3e09ed)
--
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]
