bito-code-review[bot] commented on code in PR #43370:
URL: https://github.com/apache/superset/pull/43370#discussion_r3826672315
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -107,6 +108,149 @@ interface TableSize {
height: number;
}
+type ConditionalFormattingColors = {
+ backgroundColor?: string;
+ color?: string;
+ backgroundColorCellBar?: string;
+ skipValueRange?: boolean;
+};
+
+function getConditionalFormattingColors(
+ columnColorFormatters: ColorFormatters,
+ record: DataRecord,
+ columnKey: string,
+ value: DataRecordValue,
+ applyCellBars = false,
+): ConditionalFormattingColors {
+ const colors: ConditionalFormattingColors = {};
+ const applyFormatter = (
+ formatter: ColorFormatters[number],
+ valueToFormat: DataRecordValue,
+ ) => {
+ const formatterResult = formatter.getColorFromValue(
+ valueToFormat as number | string | boolean | null,
+ );
+ if (!formatterResult) return;
+
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR ||
+ formatter.toTextColor
+ ) {
+ colors.color = formatterResult;
+ } else if (formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR) {
+ if (applyCellBars) {
+ colors.backgroundColorCellBar = forceHexAlpha(formatterResult);
+ }
+ } else {
+ colors.backgroundColor = formatterResult;
+ colors.skipValueRange = true;
+ }
+ };
+
+ columnColorFormatters
+ .filter(formatter => {
+ if (formatter.columnFormatting) {
+ return formatter.columnFormatting === columnKey;
+ }
+ return formatter.column === columnKey;
+ })
+ .forEach(formatter => {
+ const valueToFormat = formatter.columnFormatting
+ ? record[formatter.column as string]
+ : value;
+ applyFormatter(formatter, valueToFormat);
+ });
+
+ columnColorFormatters
+ .filter(
+ formatter =>
+ formatter.columnFormatting === ObjectFormattingEnum.ENTIRE_ROW,
+ )
+ .forEach(formatter =>
+ applyFormatter(formatter, record[formatter.column as string]),
+ );
+
+ return colors;
+}
+
+function lookupRowBasicColor(
+ formatters: { [key: string]: BasicColorFormatterType } | undefined,
+ columnKey: string,
+ originKey: string,
+) {
+ return (
+ formatters?.[columnKey] || (originKey ? formatters?.[originKey] :
undefined)
+ );
+}
+
+function resolveCellColorFormatting({
+ hasColumnColorFormatters,
+ columnColorFormatters,
+ record,
+ columnKey,
+ value,
+ applyCellBars,
+ comparisonColorFormatters,
+ greenRedFormatters,
+ originKey,
+}: {
+ hasColumnColorFormatters: boolean;
+ columnColorFormatters?: ColorFormatters;
+ record: DataRecord;
+ columnKey: string;
+ value: DataRecordValue;
+ applyCellBars: boolean;
+ comparisonColorFormatters?: { [key: string]: BasicColorFormatterType };
+ greenRedFormatters?: { [key: string]: BasicColorFormatterType };
+ originKey: string;
+}): ConditionalFormattingColors {
+ const colors: ConditionalFormattingColors = {};
+
+ if (!hasColumnColorFormatters) {
+ const comparison = lookupRowBasicColor(
+ comparisonColorFormatters,
+ columnKey,
+ originKey,
+ );
+ if (comparison?.backgroundColor) {
+ colors.backgroundColor = comparison.backgroundColor;
+ }
+ }
+
+ if (hasColumnColorFormatters && columnColorFormatters) {
+ const formatting = getConditionalFormattingColors(
+ columnColorFormatters,
+ record,
+ columnKey,
+ value,
+ applyCellBars,
+ );
+ if (formatting.color) {
+ colors.color = formatting.color;
+ }
+ if (formatting.backgroundColor) {
+ colors.backgroundColor = formatting.backgroundColor;
+ }
+ if (formatting.backgroundColorCellBar) {
+ colors.backgroundColorCellBar = formatting.backgroundColorCellBar;
+ }
+ if (formatting.skipValueRange) {
+ colors.skipValueRange = formatting.skipValueRange;
+ }
+ }
+
+ const greenRed = lookupRowBasicColor(
+ greenRedFormatters,
+ columnKey,
+ originKey,
+ );
+ if (greenRed?.backgroundColor) {
+ colors.backgroundColor = greenRed.backgroundColor;
+ }
+
+ return colors;
+}
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing unit tests for new color-formatting
helpers</b></div>
<div id="fix">
The three new pure helper functions — `getConditionalFormattingColors` (line
118), `lookupRowBasicColor` (line 176), and `resolveCellColorFormatting` (line
186) — collectively contain 4 conditional branches and 2 filtering paths with
no dedicated unit tests. Per [rule 6262](https://bito.aiadaptive/rule/6262),
these should be tested in isolation at the unit level, not solely through the
integration-style `TableChart.test.tsx` render tests.
</div>
</div>
<small><i>Code Review Run #d79b5e</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
