Copilot commented on code in PR #35897:
URL: https://github.com/apache/superset/pull/35897#discussion_r2773057974
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -326,6 +266,20 @@ export const FormattingPopoverContent = ({
const [column, setColumn] = useState<string>(
config?.column || columns[0]?.value,
);
+ const visibleAllColomns = useMemo(
+ () => !!(allColumns && Array.isArray(allColumns) && allColumns.length),
+ [allColumns],
+ );
+
+ const [columnFormatting, setColumnFormating] = useState<string>(
+ config?.columnFormatting ||
+ allColumns.filter(item => item.value === column)[0]?.value,
+ );
+
+ const [objectFormatting, setObjectFormating] = useState<string>(
Review Comment:
Type mismatch: objectFormatting state is typed as string but should be typed
as ObjectFormattingEnum. The same applies to columnFormatting state. This
reduces type safety since these fields should only contain specific enum
values. Consider using proper enum types: `useState<ObjectFormattingEnum>` and
`useState<string | ObjectFormattingEnum>`.
```suggestion
const [columnFormatting, setColumnFormating] = useState<
string | ObjectFormattingEnum
>(
config?.columnFormatting ||
allColumns.filter(item => item.value === column)[0]?.value,
);
const [objectFormatting, setObjectFormating] =
useState<ObjectFormattingEnum>(
```
##########
superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx:
##########
@@ -781,12 +782,16 @@ const config: ControlPanelConfig = {
item.colorScheme &&
!['Green', 'Red'].includes(item.colorScheme)
) {
- if (!item.toAllRow || !item.toTextColor) {
+ if (item.columnFormatting === undefined) {
// eslint-disable-next-line no-param-reassign
array[index] = {
...item,
- toAllRow: item.toAllRow ?? false,
- toTextColor: item.toTextColor ?? false,
+ ...(item.toTextColor === true && {
+ objectFormatting:
ObjectFormattingEnum.TEXT_COLOR,
+ }),
+ ...(item.toAllRow === true && {
+ columnFormatting:
ObjectFormattingEnum.ENTIRE_ROW,
+ }),
};
}
Review Comment:
The migration logic only applies when colorScheme is not 'Green' or 'Red',
but it should also migrate configurations that have no colorScheme set.
Additionally, when neither toTextColor nor toAllRow is true, the code doesn't
set default values for objectFormatting and columnFormatting, which could lead
to inconsistent state. Consider setting default values (BACKGROUND_COLOR for
objectFormatting) when migrating old configurations without these flags.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -409,6 +399,45 @@ export const FormattingPopoverContent = ({
</FormItem>
</Col>
</Row>
+ {visibleAllColomns && showOperatorFields ? (
+ <Row gutter={12}>
+ <Col span={12}>
+ <FormItem
+ name="columnFormatting"
+ label={t('Formatting column')}
+ rules={rulesRequired}
+ initialValue={columnFormatting}
+ >
+ <Select
+ ariaLabel={t('Select column name')}
+ options={getColumnOptions()}
+ onChange={value => {
+ handleAllColumnChange(value as string);
+ }}
+ />
+ </FormItem>
+ </Col>
+ <Col span={12}>
+ <FormItem
+ name="objectFormatting"
+ label={t('Formatting object')}
+ rules={rulesRequired}
+ initialValue={objectFormatting}
+ tooltip={t(
+ 'The background of the histogram columns is displayed if the
"Show cell bars" flag is enabled.',
Review Comment:
The tooltip text "The background of the histogram columns is displayed if
the 'Show cell bars' flag is enabled" is confusing. It only applies to the
CELL_BAR option but appears for all formatting object options. Consider making
the tooltip conditional on the selected objectFormatting value or clarify that
it only applies to cell bar formatting.
```suggestion
'Applies only when "Cell bars" formatting is selected: the
background of the histogram columns is displayed if the "Show cell bars" flag
is enabled.',
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -935,18 +939,46 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
formatter.getColorFromValue(valueToFormat);
if (!formatterResult) return;
- if (formatter.toTextColor) {
+ if (
+ formatter.objectFormatting === ObjectFormattingEnum.TEXT_COLOR
+ ) {
color = formatterResult.slice(0, -2);
+ } else if (
+ formatter.objectFormatting === ObjectFormattingEnum.CELL_BAR
+ ) {
+ if (showCellBars)
+ backgroundColorCellBar = formatterResult.slice(0, -2);
} else {
backgroundColor = formatterResult;
+ valueRangeFlag = false;
}
};
columnColorFormatters
- .filter(formatter => formatter.column === column.key)
- .forEach(formatter => applyFormatter(formatter, value));
+ .filter(formatter => {
+ if (formatter.columnFormatting) {
+ return formatter.columnFormatting === column.key;
+ }
+ return formatter.column === column.key;
+ })
+ .forEach(formatter => {
+ let valueToFormat;
+ if (formatter.columnFormatting) {
+ const index = Object.keys(row.original).findIndex(
+ key => key === formatter.column,
+ );
+ valueToFormat = row.values[index];
Review Comment:
When columnFormatting equals column.key for a specific column, the code
retrieves the value from row.values using an index found by searching
Object.keys(row.original). However, this assumes the order of keys in
row.original matches the order of values in row.values, which may not always be
true, especially with different JavaScript engine implementations. Consider
using row.original[formatter.column] directly instead of relying on index
matching, or ensure the mapping is reliable.
```suggestion
valueToFormat = row.original[formatter.column];
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -326,6 +266,20 @@ export const FormattingPopoverContent = ({
const [column, setColumn] = useState<string>(
config?.column || columns[0]?.value,
);
+ const visibleAllColomns = useMemo(
+ () => !!(allColumns && Array.isArray(allColumns) && allColumns.length),
+ [allColumns],
+ );
+
+ const [columnFormatting, setColumnFormating] = useState<string>(
+ config?.columnFormatting ||
+ allColumns.filter(item => item.value === column)[0]?.value,
Review Comment:
The initial state for columnFormatting can fail when allColumns is empty or
undefined. The expression `allColumns.filter(item => item.value ===
column)[0]?.value` will throw an error if allColumns is not defined. Consider
providing a safer default or ensuring allColumns is always defined before this
initialization. This could cause a runtime error when creating a new
conditional formatting rule.
```suggestion
config?.columnFormatting ??
(Array.isArray(allColumns)
? allColumns.find(item => item.value === column)?.value
: undefined),
```
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -969,6 +1001,7 @@ export default function TableChart<D extends DataRecord =
DataRecord>(
text-align: ${sharedStyle.textAlign};
white-space: ${value instanceof Date ? 'nowrap' : undefined};
position: relative;
+ font-weight: ${color ? `bold` : 'normal'};
Review Comment:
Adding font-weight: bold to all cells with text color formatting may not be
desirable in all cases. This is a stylistic change that wasn't mentioned in the
PR description and could affect the visual appearance of existing conditional
formatting rules. Consider making this configurable or removing it if it's not
essential to the feature.
```suggestion
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -326,6 +266,20 @@ export const FormattingPopoverContent = ({
const [column, setColumn] = useState<string>(
config?.column || columns[0]?.value,
);
+ const visibleAllColomns = useMemo(
Review Comment:
Typo in variable name: "visibleAllColomns" should be "visibleAllColumns".
This typo is used throughout the code and should be corrected for consistency.
```suggestion
const visibleAllColumns = useMemo(
```
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx:
##########
@@ -169,41 +163,6 @@ test('does not display the input fields when selected a
boolean type operator',
expect(await screen.queryByLabelText('Target value')).toBeNull();
});
Review Comment:
Tests for the toAllRow and toTextColor functionality were removed, but no
new tests were added to cover the replacement functionality (columnFormatting
and objectFormatting fields). Consider adding tests to verify: 1) the new
formatting column and formatting object selects are displayed when allColumns
is provided, 2) the cell bar option only shows numeric columns, 3) switching to
cell bar formatting automatically selects a numeric column if needed, and 4)
the getColumnOptions function returns the correct options based on
objectFormatting.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -409,6 +399,45 @@ export const FormattingPopoverContent = ({
</FormItem>
</Col>
</Row>
+ {visibleAllColomns && showOperatorFields ? (
Review Comment:
Typo in variable name: "visibleAllColomns" should be "visibleAllColumns".
This is the usage of the incorrectly spelled variable.
```suggestion
{visibleAllColumns && showOperatorFields ? (
```
--
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]
