Copilot commented on code in PR #35897:
URL: https://github.com/apache/superset/pull/35897#discussion_r2524533356
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -776,12 +776,23 @@ const config: ControlPanelConfig = {
item.colorScheme &&
!['Green', 'Red'].includes(item.colorScheme)
) {
- if (!item.toAllRow || !item.toTextColor) {
+ if (
+ !item.toAllRow ||
+ !item.toTextColor ||
+ !item.toCellBar
Review Comment:
The condition `!item.toAllRow || !item.toTextColor || !item.toCellBar` will
be true when any of these fields are `false` (not just `undefined`), because
`!false === true`. This means the block will execute even when the fields are
explicitly set to `false`, which is incorrect.
The condition should check for `undefined` explicitly:
```typescript
if (
item.toAllRow === undefined ||
item.toTextColor === undefined ||
item.toCellBar === undefined
) {
```
This matches the conditional spread pattern used inside the block which
correctly checks for `=== undefined`.
```suggestion
item.toAllRow === undefined ||
item.toTextColor === undefined ||
item.toCellBar === undefined
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -414,6 +427,26 @@ export const FormattingPopoverContent = ({
</Col>
</Row>
)}
+ {showOperatorFields && columnTypeNumeric && showToCellBar && (
+ <Row gutter={20}>
+ <Col span={1}>
+ <FormItem
+ name="toCellBar"
+ valuePropName="checked"
+ initialValue={toCellBar}
+ >
+ <Checkbox
+ onChange={event => setToCellBar(event.target.checked)}
+ checked={toCellBar}
+ disabled={toTextColor || toAllRow}
+ />
Review Comment:
When checking `toCellBar`, consider automatically unchecking `toTextColor`
and `toAllRow` to enforce mutual exclusivity at the state level, not just at
the UI level through the `disabled` prop. Currently, if a user has
`toTextColor` checked and then checks `toCellBar`, both flags could be true
when the form is submitted.
Add state synchronization:
```typescript
onChange={event => {
const checked = event.target.checked;
setToCellBar(checked);
if (checked) {
setToTextColor(false);
setToAllRow(false);
}
}}
```
Similarly, when checking `toTextColor` or `toAllRow`, they should uncheck
`toCellBar`.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -406,6 +418,7 @@ export const FormattingPopoverContent = ({
<Checkbox
onChange={event => setToTextColor(event.target.checked)}
Review Comment:
When checking `toTextColor`, consider automatically unchecking `toCellBar`
to enforce mutual exclusivity at the state level. Currently, if a user has
`toCellBar` checked and then checks `toTextColor`, both flags could be true
when the form is submitted.
Add state synchronization:
```typescript
onChange={event => {
const checked = event.target.checked;
setToTextColor(checked);
if (checked) {
setToCellBar(false);
}
}}
```
```suggestion
onChange={event => {
const checked = event.target.checked;
setToTextColor(checked);
if (checked) {
setToCellBar(false);
}
}}
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -387,6 +398,7 @@ export const FormattingPopoverContent = ({
<Checkbox
onChange={event => setToAllRow(event.target.checked)}
Review Comment:
When checking `toAllRow`, consider automatically unchecking `toCellBar` to
enforce mutual exclusivity at the state level. Currently, if a user has
`toCellBar` checked and then checks `toAllRow`, both flags could be true when
the form is submitted.
Add state synchronization:
```typescript
onChange={event => {
const checked = event.target.checked;
setToAllRow(checked);
if (checked) {
setToCellBar(false);
}
}}
```
```suggestion
onChange={event => {
const checked = event.target.checked;
setToAllRow(checked);
if (checked) {
setToCellBar(false);
}
}}
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -883,8 +884,12 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
if (formatter.toTextColor) {
color = formatterResult.slice(0, -2);
+ } else if (formatter.toCellBar) {
+ if (showCellBars)
+ backgroundColorCellBar = formatterResult.slice(0, -2);
Review Comment:
The check `if (showCellBars)` inside the `toCellBar` formatter logic may
cause the cell bar color formatting to not be applied when `showCellBars` is
false, even though the user has explicitly enabled `toCellBar` formatting for
this specific conditional format.
Consider whether the `toCellBar` formatting should respect the global
`showCellBars` setting or if it should override it when explicitly enabled. If
it should override, remove the `if (showCellBars)` condition. If it should
respect it, the current implementation is correct but might be confusing to
users who enable `toCellBar` but don't see any effect when cell bars are
globally disabled.
```suggestion
backgroundColorCellBar = formatterResult.slice(0, -2);
```
--
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]
