Copilot commented on code in PR #43692:
URL: https://github.com/apache/superset/pull/43692#discussion_r3890288593
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -242,45 +242,51 @@ export const FormattingPopoverContent = ({
}) => {
const [form] = Form.useForm();
const colors = colorScheme();
- const [showOperatorFields, setShowOperatorFields] = useState(
- config === undefined ||
- (config?.colorScheme !== ColorSchemeEnum.Green &&
- config?.colorScheme !== ColorSchemeEnum.Red),
+ // Mirrors the colorScheme form field; trend color schemes hide the
+ // operator fields, the fallback applies before the field is registered
+ const colorSchemeValue = Form.useWatch('colorScheme', form);
+ const showOperatorFields =
+ colorSchemeValue !== ColorSchemeEnum.Green &&
+ colorSchemeValue !== ColorSchemeEnum.Red;
+
+ // Mirrors the column form field; the form store is the source of truth,
+ // the fallback applies before the field is registered
+ const column = Form.useWatch('column', form) ?? columns[0]?.value;
Review Comment:
`showOperatorFields` is derived from `Form.useWatch('colorScheme', form)`,
but `useWatch` can be `undefined` on the initial render even when
`config.colorScheme` is a trend color. That can briefly render
operator/formatting fields before the form store is initialized (and the inline
comment says there is a fallback, but none is implemented). Consider falling
back to `config?.colorScheme` (and similarly for `column`) to keep the UI
stable for pre-populated configs.
##########
superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx:
##########
@@ -332,40 +336,77 @@ export const FormattingPopoverContent = ({
);
const handleObjectChange = (value: ObjectFormattingEnum) => {
- setObjectFormatting(value);
-
if (value === ObjectFormattingEnum.CELL_BAR) {
const currentColumnValue = form.getFieldValue('columnFormatting');
const isCurrentColumnNumeric = numericColumns.some(
col => col.value === currentColumnValue,
);
- if (!isCurrentColumnNumeric && numericColumns.length > 0) {
+ if (
+ !isCurrentColumnNumeric &&
+ numericColumns.length > 0 &&
+ !applyToWholeRow
+ ) {
const newValue = numericColumns[0]?.value || '';
form.setFieldsValue({
columnFormatting: newValue,
});
- setColumnFormatting(newValue);
}
}
};
- const getColumnOptions = useCallback(
- () =>
+ const handleWholeRowChange = (checked: boolean) => {
+ setApplyToWholeRow(checked);
+ // The field is kept in sync for display (the disabled select shows the
+ // "entire row" label); handleFinish derives the submitted value from
+ // the checkbox
+ const currentColumn = form.getFieldValue('columnFormatting');
+ if (checked) {
+ if (currentColumn !== ObjectFormattingEnum.ENTIRE_ROW) {
+ previousColumnRef.current = currentColumn;
+ }
+ form.setFieldsValue({
+ columnFormatting: ObjectFormattingEnum.ENTIRE_ROW,
+ });
+ } else {
+ form.setFieldsValue({
+ columnFormatting:
+ previousColumnRef.current ??
+ allColumns.find(item => item.value === column)?.value,
+ });
+ }
Review Comment:
When unchecking "Apply for whole row", the fallback `allColumns.find(item =>
item.value === column)?.value` can still resolve to `undefined` (e.g. if the
selected condition `column` isn’t present in `allColumns`). That would leave
`columnFormatting` unset and block form submission due to the required rule.
Add a final fallback (e.g. the first regular column) to guarantee a valid
selection.
--
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]
