bito-code-review[bot] commented on code in PR #42053:
URL: https://github.com/apache/superset/pull/42053#discussion_r3731532024
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts:
##########
@@ -25,14 +25,15 @@ import {
ObjectFormattingEnum,
} from '@superset-ui/chart-controls';
import { GenericDataType } from '@apache-superset/core/common';
+import { type RGBColor } from '@superset-ui/core/components';
export type ConditionalFormattingConfig = {
operator?: Comparator;
targetValue?: number;
targetValueLeft?: number;
targetValueRight?: number;
column?: string;
- colorScheme?: string;
+ colorScheme?: RGBColor | string;
Review Comment:
<!-- Bito Reply -->
The suggestion identifies a potential runtime issue where passing an
`RGBColor` object to `calculateBasicStyle` will cause comparison failures, as
that function expects a `ColorSchemeEnum` string. Applying this suggestion is
appropriate if the intention is to support custom RGB colors, but it requires a
corresponding update to the consumer logic in `transformProps.ts` to handle the
new `RGBColor` type safely.
**superset-frontend/src/explore/components/controls/ConditionalFormattingControl/types.ts**
```
column?: string;
- colorScheme?: string;
+ colorScheme?: RGBColor | string;
```
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx:
##########
@@ -202,7 +202,7 @@ export function getCellColor(
for (const formatter of cellColorFormatter) {
if (formatter.column === key) {
const result = formatter.getColorFromValue(aggValue);
- if (result) {
+ if (result && typeof result === 'string') {
Review Comment:
<!-- Bito Reply -->
The suggestion to remove the `typeof result === 'string'` type guard is
appropriate. As noted in the review, `getColorFromValue` is already typed to
return `string | undefined`, and the existing codebase consistently uses `if
(result)` for similar checks, making the explicit type guard redundant and
inconsistent with established patterns.
**superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.tsx**
```
const result = formatter.getColorFromValue(aggValue);
if (result) {
```
--
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]
