diff --git a/superset-frontend/packages/superset-core/src/ui/theme/types.ts b/superset-frontend/packages/superset-core/src/ui/theme/types.ts index ac905393f9a..a0b30ca5956 100644 --- a/superset-frontend/packages/superset-core/src/ui/theme/types.ts +++ b/superset-frontend/packages/superset-core/src/ui/theme/types.ts @@ -116,6 +116,7 @@ export interface SupersetSpecificTokens { fontWeightNormal: string; fontWeightLight: string; fontWeightStrong: number; + fontWeightBold: string; // Brand-related brandIconMaxWidth: number; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts index 25aaa81b30b..b501b7c9bff 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/types.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/types.ts @@ -491,12 +491,16 @@ export type ConditionalFormattingConfig = { toAllRow?: boolean; toTextColor?: boolean; useGradient?: boolean; + columnFormatting?: string; + objectFormatting?: ObjectFormattingEnum; }; export type ColorFormatters = { column: string; toAllRow?: boolean; toTextColor?: boolean; + columnFormatting?: string; + objectFormatting?: ObjectFormattingEnum; getColorFromValue: ( value: number | string | boolean | null, ) => string | undefined; @@ -614,6 +618,13 @@ export type ControlFormItemSpec = { } : {}); +export enum ObjectFormattingEnum { + BACKGROUND_COLOR = 'BACKGROUND_COLOR', + TEXT_COLOR = 'TEXT_COLOR', + CELL_BAR = 'CELL_BAR', + ENTIRE_ROW = 'ENTIRE_ROW', +} + export enum ColorSchemeEnum { Green = 'Green', Red = 'Red', diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts index dbfa4b28428..81d1ee40a42 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts +++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/getColorFormatters.ts @@ -311,6 +311,8 @@ export const getColorFormatters = memoizeOne( column: config?.column, toAllRow: config?.toAllRow, toTextColor: config?.toTextColor, + columnFormatting: config?.columnFormatting, + objectFormatting: config?.objectFormatting, getColorFromValue: getColorFunction( { ...config, colorScheme: resolvedColorScheme }, data.map(row => row[config.column!] as number), diff --git a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx index 9aae0231f51..499ae52e952 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx @@ -74,7 +74,11 @@ import { TableOutlined, } from '@ant-design/icons'; import { isEmpty, debounce, isEqual } from 'lodash'; -import { ColorFormatters, ColorSchemeEnum } from '@superset-ui/chart-controls'; +import { + ColorFormatters, + ObjectFormattingEnum, + ColorSchemeEnum, +} from '@superset-ui/chart-controls'; import { DataColumnMeta, SearchOption, @@ -874,12 +878,11 @@ export default function TableChart( isUsingTimeComparison && Array.isArray(basicColorFormatters) && basicColorFormatters.length > 0; + const generalShowCellBars = + config.showCellBars === undefined ? showCellBars : config.showCellBars; const valueRange = !hasBasicColorFormatters && - !hasColumnColorFormatters && - (config.showCellBars === undefined - ? showCellBars - : config.showCellBars) && + generalShowCellBars && (isMetric || isRawRecords || isPercentMetric) && getValueRange(key, alignPositiveNegative); @@ -914,6 +917,8 @@ export default function TableChart( let backgroundColor; let color; + let backgroundColorCellBar; + let valueRangeFlag = true; let arrow = ''; const originKey = column.key.substring(column.label.length).trim(); if (!hasColumnColorFormatters && hasBasicColorFormatters) { @@ -934,18 +939,43 @@ export default function TableChart( 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 (generalShowCellBars) + 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) { + valueToFormat = row.original[formatter.column]; + } else { + valueToFormat = value; + } + applyFormatter(formatter, valueToFormat); + }); columnColorFormatters - .filter(formatter => formatter.toAllRow) + .filter( + formatter => + formatter.columnFormatting === + ObjectFormattingEnum.ENTIRE_ROW, + ) .forEach(formatter => applyFormatter(formatter, row.original[formatter.column]), ); @@ -968,6 +998,9 @@ export default function TableChart( text-align: ${sharedStyle.textAlign}; white-space: ${value instanceof Date ? 'nowrap' : undefined}; position: relative; + font-weight: ${color + ? `${theme.fontWeightBold}` + : `${theme.fontWeightNormal}`}; background: ${backgroundColor || undefined}; padding-left: ${column.isChildColumn ? `${theme.sizeUnit * 5}px` @@ -981,6 +1014,7 @@ export default function TableChart( top: 0; ${valueRange && typeof value === 'number' && + valueRangeFlag && ` width: ${`${cellWidth({ value: value as number, @@ -992,11 +1026,14 @@ export default function TableChart( valueRange, alignPositiveNegative, })}%`}; - background-color: ${cellBackground({ - value: value as number, - colorPositiveNegative, - theme, - })}; + background-color: ${ + (backgroundColorCellBar && `${backgroundColorCellBar}99`) || + cellBackground({ + value: value as number, + colorPositiveNegative, + theme, + }) + }; `} `; diff --git a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx index 3084ce69a52..d7417ee78b4 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/superset-frontend/plugins/plugin-chart-table/src/controlPanel.tsx @@ -40,6 +40,7 @@ import { isRegularMetric, isPercentMetric, ConditionalFormattingConfig, + ObjectFormattingEnum, ColorSchemeEnum, } from '@superset-ui/chart-controls'; import { t } from '@apache-superset/core'; @@ -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, + }), }; } } @@ -795,6 +800,23 @@ const config: ControlPanelConfig = { } const { colnames, coltypes } = chart?.queriesResponse?.[0] ?? {}; + const allColumns = + Array.isArray(colnames) && Array.isArray(coltypes) + ? [ + { + value: ObjectFormattingEnum.ENTIRE_ROW, + label: t('entire row'), + dataType: GenericDataType.String, + }, + ...colnames.map((colname: string, index: number) => ({ + value: colname, + label: Array.isArray(verboseMap) + ? colname + : (verboseMap[colname] ?? colname), + dataType: coltypes[index], + })), + ] + : []; const numericColumns = Array.isArray(colnames) && Array.isArray(coltypes) ? colnames.reduce((acc, colname, index) => { @@ -826,10 +848,7 @@ const config: ControlPanelConfig = { removeIrrelevantConditions: chartStatus === 'success', columnOptions, verboseMap, - conditionalFormattingFlag: { - toAllRowCheck: true, - toColorTextCheck: true, - }, + allColumns, extraColorChoices, }; }, diff --git a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx index 3d9da78b875..5183e5ab543 100644 --- a/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx +++ b/superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx @@ -17,6 +17,7 @@ * under the License. */ import '@testing-library/jest-dom'; +import { ObjectFormattingEnum } from '@superset-ui/chart-controls'; import { render, screen, @@ -1250,7 +1251,7 @@ describe('plugin-chart-table', () => { column: 'sum__num', operator: '>', targetValue: 2467, - toAllRow: true, + columnFormatting: ObjectFormattingEnum.ENTIRE_ROW, }, ], }, @@ -1286,7 +1287,7 @@ describe('plugin-chart-table', () => { column: 'sum__num', operator: '>', targetValue: 2467, - toTextColor: true, + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, }, ], }, @@ -1319,8 +1320,8 @@ describe('plugin-chart-table', () => { column: 'sum__num', operator: '>', targetValue: 2467, - toAllRow: true, - toTextColor: true, + columnFormatting: ObjectFormattingEnum.ENTIRE_ROW, + objectFormatting: ObjectFormattingEnum.TEXT_COLOR, }, ], }, diff --git a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.tsx b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.tsx index 54adad53514..866e101699d 100644 --- a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.tsx +++ b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/ConditionalFormattingControl.tsx @@ -74,7 +74,7 @@ const ConditionalFormattingControl = ({ verboseMap, removeIrrelevantConditions, extraColorChoices, - conditionalFormattingFlag, + allColumns, ...props }: ConditionalFormattingControlProps) => { const [conditionalFormattingConfigs, setConditionalFormattingConfigs] = @@ -159,6 +159,7 @@ const ConditionalFormattingControl = ({ } destroyTooltipOnHide extraColorChoices={extraColorChoices} + allColumns={allColumns} > @@ -175,7 +176,7 @@ const ConditionalFormattingControl = ({ onChange={onSave} destroyTooltipOnHide extraColorChoices={extraColorChoices} - conditionalFormattingFlag={conditionalFormattingFlag} + allColumns={allColumns} > { const [visible, setVisible] = useState(false); @@ -50,7 +50,7 @@ export const FormattingPopover = ({ config={config} columns={columns} extraColorChoices={extraColorChoices} - conditionalFormattingFlag={conditionalFormattingFlag} + allColumns={allColumns} /> } open={visible} diff --git a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx index 4957744b3b8..232661f627b 100644 --- a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx +++ b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.test.tsx @@ -25,7 +25,6 @@ import { import { Comparator, ColorSchemeEnum } from '@superset-ui/chart-controls'; import { GenericDataType } from '@apache-superset/core/api/core'; import { FormattingPopoverContent } from './FormattingPopoverContent'; -import { ConditionalFormattingConfig } from './types'; const mockOnChange = jest.fn(); @@ -44,6 +43,12 @@ const columnsBooleanType = [ { label: 'Column 2', value: 'column2', dataType: GenericDataType.Boolean }, ]; +const mixColumns = [ + { label: 'Name', value: 'name', dataType: GenericDataType.String }, + { label: 'Sales', value: 'sales', dataType: GenericDataType.Numeric }, + { label: 'Active', value: 'active', dataType: GenericDataType.Boolean }, +]; + const extraColorChoices = [ { value: ColorSchemeEnum.Green, @@ -55,11 +60,6 @@ const extraColorChoices = [ }, ]; -const config: ConditionalFormattingConfig = { - toAllRow: true, - toTextColor: true, -}; - test('renders FormattingPopoverContent component', () => { render( { +test('displays Use gradient checkbox', () => { render( , ); - expect(screen.getByText('To entire row')).toBeInTheDocument(); - expect(screen.getByText('To text color')).toBeInTheDocument(); -}); - -test('displays the toAllRow and toTextColor flags based on the selected string type operator', () => { - render( - , - ); - - expect(screen.getByText('To entire row')).toBeInTheDocument(); - expect(screen.getByText('To text color')).toBeInTheDocument(); -}); - -test('Not displays the toAllRow and toTextColor flags', () => { - render( - , - ); - - expect(screen.queryByText('To entire row')).not.toBeInTheDocument(); - expect(screen.queryByText('To text color')).not.toBeInTheDocument(); -}); - -test('displays Use gradient checkbox', () => { - render( - , - ); - expect(screen.getByText('Use gradient')).toBeInTheDocument(); }); @@ -229,7 +198,11 @@ const findUseGradientCheckbox = (): HTMLInputElement => { test('Use gradient checkbox defaults to checked', () => { render( - , + , ); const checkbox = findUseGradientCheckbox(); @@ -238,7 +211,11 @@ test('Use gradient checkbox defaults to checked', () => { test('Use gradient checkbox can be toggled', async () => { render( - , + , ); const checkbox = findUseGradientCheckbox(); @@ -252,3 +229,81 @@ test('Use gradient checkbox can be toggled', async () => { fireEvent.click(checkbox); expect(checkbox).toBeChecked(); }); + +test('The Use Gradient check box is not displayed for string and boolean and is displayed for numeric data types.', () => { + render( + , + ); + + expect(screen.queryByText('Use gradient')).not.toBeInTheDocument(); + + render( + , + ); + + expect(screen.queryByText('Use gradient')).not.toBeInTheDocument(); + + render( + , + ); + + expect(screen.queryByText('Use gradient')).toBeInTheDocument(); +}); + +test('should display formatting column and object fields when allColumns is provided and non-empty', async () => { + render( + , + ); + + await waitFor(() => { + expect(screen.getByText('Formatting column')).toBeInTheDocument(); + expect(screen.getByText('Formatting object')).toBeInTheDocument(); + }); +}); + +test('should hide formatting fields when allColumns is empty', async () => { + render( + , + ); + + await waitFor(() => { + expect(screen.queryByText('Formatting column')).not.toBeInTheDocument(); + expect(screen.queryByText('Formatting object')).not.toBeInTheDocument(); + }); +}); + +test('should hide formatting fields when color scheme is Green', async () => { + render( + , + ); + + await waitFor(() => { + expect(screen.queryByText('Formatting column')).not.toBeInTheDocument(); + expect(screen.queryByText('Formatting object')).not.toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx index 85e3624558d..7331cfbb9a4 100644 --- a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx +++ b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx @@ -16,13 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { useMemo, useState, useEffect } from 'react'; +import { useMemo, useState, useEffect, useCallback } from 'react'; import { t } from '@apache-superset/core'; import { styled } from '@apache-superset/core/ui'; import { GenericDataType } from '@apache-superset/core/api/core'; import { Comparator, MultipleValueComparators, + ObjectFormattingEnum, ColorSchemeEnum, } from '@superset-ui/chart-controls'; import { @@ -37,10 +38,14 @@ import { Checkbox, type FormProps, } from '@superset-ui/core/components'; +import { ConditionalFormattingConfig, ColumnOption } from './types'; import { - ConditionalFormattingConfig, - ConditionalFormattingFlag, -} from './types'; + operatorOptions, + stringOperatorOptions, + booleanOperatorOptions, + formattingOptions, + colorSchemeOptions, +} from './constants'; const FullWidthInputNumber = styled(InputNumber)` width: 100%; @@ -55,43 +60,6 @@ const JustifyEnd = styled.div` justify-content: flex-end; `; -// Use theme token names instead of hex values to support theme switching -const colorSchemeOptions = () => [ - { value: 'colorSuccess', label: t('success') }, - { value: 'colorWarning', label: t('alert') }, - { value: 'colorError', label: t('error') }, -]; - -const operatorOptions = [ - { value: Comparator.None, label: t('None') }, - { value: Comparator.GreaterThan, label: '>' }, - { value: Comparator.LessThan, label: '<' }, - { value: Comparator.GreaterOrEqual, label: '≥' }, - { value: Comparator.LessOrEqual, label: '≤' }, - { value: Comparator.Equal, label: '=' }, - { value: Comparator.NotEqual, label: '≠' }, - { value: Comparator.Between, label: '< x <' }, - { value: Comparator.BetweenOrEqual, label: '≤ x ≤' }, - { value: Comparator.BetweenOrLeftEqual, label: '≤ x <' }, - { value: Comparator.BetweenOrRightEqual, label: '< x ≤' }, -]; - -const stringOperatorOptions = [ - { value: Comparator.None, label: t('None') }, - { value: Comparator.Equal, label: '=' }, - { value: Comparator.BeginsWith, label: t('begins with') }, - { value: Comparator.EndsWith, label: t('ends with') }, - { value: Comparator.Containing, label: t('containing') }, - { value: Comparator.NotContaining, label: t('not containing') }, -]; - -const booleanOperatorOptions = [ - { value: Comparator.IsNull, label: t('is null') }, - { value: Comparator.IsTrue, label: t('is true') }, - { value: Comparator.IsFalse, label: t('is false') }, - { value: Comparator.IsNotNull, label: t('is not null') }, -]; - const targetValueValidator = ( compare: (targetValue: number, compareValue: number) => boolean, @@ -263,16 +231,13 @@ export const FormattingPopoverContent = ({ onChange, columns = [], extraColorChoices = [], - conditionalFormattingFlag = { - toAllRowCheck: false, - toColorTextCheck: false, - }, + allColumns = [], }: { config?: ConditionalFormattingConfig; onChange: (config: ConditionalFormattingConfig) => void; columns: { label: string; value: string; dataType: GenericDataType }[]; extraColorChoices?: { label: string; value: string }[]; - conditionalFormattingFlag?: ConditionalFormattingFlag; + allColumns?: ColumnOption[]; }) => { const [form] = Form.useForm(); const colorScheme = colorSchemeOptions(); @@ -282,35 +247,10 @@ export const FormattingPopoverContent = ({ config?.colorScheme !== ColorSchemeEnum.Red), ); - const [toAllRow, setToAllRow] = useState(() => Boolean(config?.toAllRow)); - const [toTextColor, setToTextColor] = useState(() => - Boolean(config?.toTextColor), - ); const [useGradient, setUseGradient] = useState(() => config?.useGradient !== undefined ? config.useGradient : true, ); - const useConditionalFormattingFlag = ( - flagKey: 'toAllRowCheck' | 'toColorTextCheck', - configKey: 'toAllRow' | 'toTextColor', - ) => - useMemo( - () => - conditionalFormattingFlag && conditionalFormattingFlag[flagKey] - ? config?.[configKey] === undefined - : config?.[configKey] !== undefined, - [conditionalFormattingFlag], // oxlint-disable-line react-hooks/exhaustive-deps - ); - - const showToAllRow = useConditionalFormattingFlag( - 'toAllRowCheck', - 'toAllRow', - ); - const showToColorText = useConditionalFormattingFlag( - 'toColorTextCheck', - 'toTextColor', - ); - const handleChange = (event: any) => { setShowOperatorFields( !(event === ColorSchemeEnum.Green || event === ColorSchemeEnum.Red), @@ -320,6 +260,23 @@ export const FormattingPopoverContent = ({ const [column, setColumn] = useState( config?.column || columns[0]?.value, ); + const visibleAllColumns = useMemo( + () => !!(allColumns && Array.isArray(allColumns) && allColumns.length), + [allColumns], + ); + + const [columnFormatting, setColumnFormatting] = useState( + config?.columnFormatting ?? + (Array.isArray(allColumns) + ? allColumns.find(item => item.value === column)?.value + : undefined), + ); + + const [objectFormatting, setObjectFormatting] = + useState( + config?.objectFormatting || formattingOptions[0].value, + ); + const [previousColumnType, setPreviousColumnType] = useState< GenericDataType | undefined >(); @@ -355,6 +312,51 @@ export const FormattingPopoverContent = ({ setPreviousColumnType(newColumnType); }; + const handleAllColumnChange = (value: string | undefined) => { + setColumnFormatting(value); + }; + const numericColumns = useMemo( + () => allColumns.filter(col => col.dataType === GenericDataType.Numeric), + [allColumns], + ); + + const visibleUseGradient = useMemo( + () => + numericColumns.length > 0 + ? numericColumns.some((col: ColumnOption) => col.value === column) && + objectFormatting === ObjectFormattingEnum.BACKGROUND_COLOR + : false, + [column, numericColumns, objectFormatting], + ); + + 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) { + const newValue = numericColumns[0]?.value || ''; + form.setFieldsValue({ + columnFormatting: newValue, + }); + setColumnFormatting(newValue); + } + } + }; + + const getColumnOptions = useCallback( + () => + objectFormatting === ObjectFormattingEnum.CELL_BAR + ? numericColumns + : allColumns, + [objectFormatting, numericColumns, allColumns], + ); + useEffect(() => { if (column && !previousColumnType) { setPreviousColumnType( @@ -403,23 +405,68 @@ export const FormattingPopoverContent = ({ - - - - setUseGradient(event.target.checked)} - checked={useGradient} - /> - - - - {t('Use gradient')} - - + {visibleAllColumns && showOperatorFields ? ( + + + + { + handleObjectChange(value); + }} + /> + + + + ) : null} + {visibleUseGradient && ( + + + + setUseGradient(event.target.checked)} + checked={useGradient} + /> + + + + {t('Use gradient')} + + + )} {showOperatorFields ? ( (props: GetFieldValue) => renderOperatorFields(props, columnType) @@ -431,47 +478,6 @@ export const FormattingPopoverContent = ({ )} - - {showOperatorFields && showToAllRow && ( - - - - setToAllRow(event.target.checked)} - checked={toAllRow} - /> - - - - {t('To entire row')} - - - )} - {showOperatorFields && showToColorText && ( - - - - setToTextColor(event.target.checked)} - checked={toTextColor} - /> - - - - {t('To text color')} - - - )} - -