korbit-ai[bot] commented on code in PR #35863:
URL: https://github.com/apache/superset/pull/35863#discussion_r2466895818
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/controlPanel.tsx:
##########
@@ -403,10 +403,21 @@ const config: ControlPanelConfig = {
renderTrigger: true,
label: t('Conditional formatting'),
description: t('Apply conditional color formatting to metrics'),
+ shouldMapStateToProps() {
+ return true;
+ },
mapStateToProps(explore, _, chart) {
- const values =
- (explore?.controls?.metrics?.value as QueryFormMetric[]) ??
+ const metrics =
+ (explore?.controls?.metrics?.value as QueryFormMetric[]) ||
[];
+ const columns =
+ (explore?.controls?.groupbyColumns
+ ?.value as QueryFormMetric[]) || [];
+ const rows =
+ (explore?.controls?.groupbyRows
+ ?.value as QueryFormMetric[]) || [];
+ const values = [...metrics, ...new Set([...columns, ...rows])];
Review Comment:
### Incomplete deduplication in values array <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Set constructor is applied to the spread of columns and rows arrays, but
the result is then spread again, which doesn't achieve deduplication across all
three arrays (metrics, columns, rows).
###### Why this matters
This will only deduplicate within the combined columns and rows arrays, but
won't remove duplicates between metrics and the groupby arrays. If the same
metric appears in both metrics and groupby arrays, it will appear twice in the
final values array, potentially causing duplicate entries in the conditional
formatting options.
###### Suggested change ∙ *Feature Preview*
Apply the Set constructor to the entire combined array to properly
deduplicate across all three sources:
```typescript
const values = [...new Set([...metrics, ...columns, ...rows])];
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b7a0b6eb-c1f4-460c-a950-512739953393/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b7a0b6eb-c1f4-460c-a950-512739953393?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b7a0b6eb-c1f4-460c-a950-512739953393?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b7a0b6eb-c1f4-460c-a950-512739953393?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b7a0b6eb-c1f4-460c-a950-512739953393)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ac9475e7-987b-4bf6-a911-9d707f8bb7de -->
[](ac9475e7-987b-4bf6-a911-9d707f8bb7de)
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/react-pivottable/TableRenderers.jsx:
##########
@@ -348,6 +348,30 @@ export class TableRenderer extends Component {
return spans;
}
+ getCellColor(keys, aggValue, cellColorFormatters) {
+ let backgroundColor;
+ if (cellColorFormatters) {
+ Object.values(cellColorFormatters).forEach(cellColorFormatter => {
+ if (Array.isArray(cellColorFormatter)) {
+ keys.forEach(key => {
+ if (backgroundColor) {
+ return;
+ }
+ cellColorFormatter
+ .filter(formatter => formatter.column === key)
+ .forEach(formatter => {
+ const formatterResult = formatter.getColorFromValue(aggValue);
+ if (formatterResult) {
+ backgroundColor = formatterResult;
+ }
+ });
Review Comment:
### Missing formatter precedence handling <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getCellColor method overwrites backgroundColor without considering
priority or precedence when multiple formatters match the same key.
###### Why this matters
This can lead to unpredictable coloring behavior where the last matching
formatter always wins, potentially hiding more important formatting rules that
should take precedence.
###### Suggested change ∙ *Feature Preview*
Implement a priority system or break early after finding the first valid
formatter result:
```javascript
cellColorFormatter
.filter(formatter => formatter.column === key)
.some(formatter => {
const formatterResult = formatter.getColorFromValue(aggValue);
if (formatterResult) {
backgroundColor = formatterResult;
return true; // Break early
}
return false;
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8099820-c6dd-4431-8de8-a3b561c55933/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8099820-c6dd-4431-8de8-a3b561c55933?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8099820-c6dd-4431-8de8-a3b561c55933?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8099820-c6dd-4431-8de8-a3b561c55933?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a8099820-c6dd-4431-8de8-a3b561c55933)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:67c65f87-9d3c-4d79-8da2-b02d81168bdc -->
[](67c65f87-9d3c-4d79-8da2-b02d81168bdc)
--
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]
