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 9134715bd363..249037a4cfa0 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 @@ -241,10 +241,21 @@ export const getColorFormatters = memoizeOne( ( columnConfig: ConditionalFormattingConfig[] | undefined, data: DataRecord[], + theme?: Record, alpha?: boolean, ) => columnConfig?.reduce( (acc: ColorFormatters, config: ConditionalFormattingConfig) => { + let resolvedColorScheme = config.colorScheme; + if ( + theme && + typeof config.colorScheme === 'string' && + config.colorScheme.startsWith('color') && + theme[config.colorScheme] + ) { + resolvedColorScheme = theme[config.colorScheme] as string; + } + if ( config?.column !== undefined && (config?.operator === Comparator.None || @@ -257,7 +268,7 @@ export const getColorFormatters = memoizeOne( acc.push({ column: config?.column, getColorFromValue: getColorFunction( - config, + { ...config, colorScheme: resolvedColorScheme }, data.map(row => row[config.column!] as number), alpha, ), diff --git a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx index 9620659d78d9..e534f920e5ed 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/ThemedAgGridReact.test.tsx @@ -218,3 +218,29 @@ test('handles missing theme gracefully', () => { // Should still render without crashing expect(screen.getByTestId('ag-grid-react')).toBeInTheDocument(); }); + +test('merges theme overrides with default theme parameters', () => { + const themeOverrides = { + fontSize: 16, + headerBackgroundColor: '#custom-color', + }; + + render( + , + ); + + const agGrid = screen.getByTestId('ag-grid-react'); + const theme = JSON.parse(agGrid.getAttribute('data-theme') || '{}'); + + // Custom overrides should be applied + expect(theme.fontSize).toBe(16); + expect(theme.headerBackgroundColor).toBe('#custom-color'); + + // Default theme parameters should still be present + expect(theme.foregroundColor).toBeDefined(); + expect(theme.borderColor).toBeDefined(); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx index 5e95a9c00020..da3fc8c59100 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/index.tsx @@ -186,5 +186,5 @@ export { // Re-export AgGridReact for ref types export { AgGridReact } from 'ag-grid-react'; -// Export the setup function for AG-Grid modules -export { setupAGGridModules } from './setupAGGridModules'; +// Export the setup function and default modules for AG-Grid +export { setupAGGridModules, defaultModules } from './setupAGGridModules'; diff --git a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.test.ts b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.test.ts new file mode 100644 index 000000000000..489c631d3564 --- /dev/null +++ b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.test.ts @@ -0,0 +1,110 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ModuleRegistry } from 'ag-grid-community'; +import { setupAGGridModules, defaultModules } from './setupAGGridModules'; + +jest.mock('ag-grid-community', () => ({ + ModuleRegistry: { + registerModules: jest.fn(), + }, + ColumnAutoSizeModule: { moduleName: 'ColumnAutoSizeModule' }, + ColumnHoverModule: { moduleName: 'ColumnHoverModule' }, + RowAutoHeightModule: { moduleName: 'RowAutoHeightModule' }, + RowStyleModule: { moduleName: 'RowStyleModule' }, + PaginationModule: { moduleName: 'PaginationModule' }, + CellStyleModule: { moduleName: 'CellStyleModule' }, + TextFilterModule: { moduleName: 'TextFilterModule' }, + NumberFilterModule: { moduleName: 'NumberFilterModule' }, + DateFilterModule: { moduleName: 'DateFilterModule' }, + ExternalFilterModule: { moduleName: 'ExternalFilterModule' }, + CsvExportModule: { moduleName: 'CsvExportModule' }, + ColumnApiModule: { moduleName: 'ColumnApiModule' }, + RowApiModule: { moduleName: 'RowApiModule' }, + CellApiModule: { moduleName: 'CellApiModule' }, + RenderApiModule: { moduleName: 'RenderApiModule' }, + ClientSideRowModelModule: { moduleName: 'ClientSideRowModelModule' }, + CustomFilterModule: { moduleName: 'CustomFilterModule' }, +})); + +beforeEach(() => { + jest.clearAllMocks(); +}); + +test('defaultModules exports an array of AG Grid modules', () => { + expect(Array.isArray(defaultModules)).toBe(true); + expect(defaultModules.length).toBeGreaterThan(0); + + // Verify it contains expected modules + const moduleNames = defaultModules.map((m: any) => m.moduleName); + expect(moduleNames).toContain('ColumnAutoSizeModule'); + expect(moduleNames).toContain('PaginationModule'); + expect(moduleNames).toContain('ClientSideRowModelModule'); +}); + +test('setupAGGridModules registers default modules when called without arguments', () => { + setupAGGridModules(); + + expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1); + expect(ModuleRegistry.registerModules).toHaveBeenCalledWith(defaultModules); +}); + +test('setupAGGridModules registers default + additional modules when provided', () => { + const mockEnterpriseModule1 = { moduleName: 'MultiFilterModule' }; + const mockEnterpriseModule2 = { moduleName: 'PivotModule' }; + const additionalModules = [mockEnterpriseModule1, mockEnterpriseModule2]; + + setupAGGridModules(additionalModules); + + expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1); + + const registeredModules = (ModuleRegistry.registerModules as jest.Mock).mock + .calls[0][0]; + + // Should contain all default modules + defaultModules.forEach(module => { + expect(registeredModules).toContain(module); + }); + + // Should contain additional modules + expect(registeredModules).toContain(mockEnterpriseModule1); + expect(registeredModules).toContain(mockEnterpriseModule2); + + // Total length should be default + additional + expect(registeredModules.length).toBe( + defaultModules.length + additionalModules.length, + ); +}); + +test('setupAGGridModules handles empty additional modules array', () => { + setupAGGridModules([]); + + expect(ModuleRegistry.registerModules).toHaveBeenCalledTimes(1); + expect(ModuleRegistry.registerModules).toHaveBeenCalledWith(defaultModules); +}); + +test('setupAGGridModules does not mutate defaultModules array', () => { + const originalLength = defaultModules.length; + const mockEnterpriseModule = { moduleName: 'EnterpriseModule' }; + + setupAGGridModules([mockEnterpriseModule]); + + // defaultModules should remain unchanged + expect(defaultModules.length).toBe(originalLength); + expect(defaultModules).not.toContain(mockEnterpriseModule); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.ts b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.ts index 6c0cc6419151..7bc39fc56dba 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/ThemedAgGridReact/setupAGGridModules.ts @@ -19,6 +19,7 @@ import { ModuleRegistry, + type Module, ColumnAutoSizeModule, ColumnHoverModule, RowAutoHeightModule, @@ -39,27 +40,29 @@ import { } from 'ag-grid-community'; /** - * Registers the AG-Grid modules required for Superset's table functionality. - * This should be called once during application initialization. + * Default AG Grid modules that are registered by default. + * These modules provide core AG Grid functionality. */ -export const setupAGGridModules = () => { - ModuleRegistry.registerModules([ - ColumnAutoSizeModule, - ColumnHoverModule, - RowAutoHeightModule, - RowStyleModule, - PaginationModule, - CellStyleModule, - TextFilterModule, - NumberFilterModule, - DateFilterModule, - ExternalFilterModule, - CsvExportModule, - ColumnApiModule, - RowApiModule, - CellApiModule, - RenderApiModule, - ClientSideRowModelModule, - CustomFilterModule, - ]); +export const defaultModules: Module[] = [ + ColumnAutoSizeModule, + ColumnHoverModule, + RowAutoHeightModule, + RowStyleModule, + PaginationModule, + CellStyleModule, + TextFilterModule, + NumberFilterModule, + DateFilterModule, + ExternalFilterModule, + CsvExportModule, + ColumnApiModule, + RowApiModule, + CellApiModule, + RenderApiModule, + ClientSideRowModelModule, + CustomFilterModule, +]; + +export const setupAGGridModules = (additionalModules: Module[] = []) => { + ModuleRegistry.registerModules([...defaultModules, ...additionalModules]); }; diff --git a/superset-frontend/packages/superset-ui-core/src/components/index.ts b/superset-frontend/packages/superset-ui-core/src/components/index.ts index 6590688076ff..e1506f642190 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/index.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/index.ts @@ -180,6 +180,7 @@ export { ThemedAgGridReact, type ThemedAgGridReactProps, setupAGGridModules, + defaultModules, } from './ThemedAgGridReact'; export { CodeEditor, diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail-dark.png b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail-dark.png index 296702c954af..919eaa2118ea 100644 Binary files a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail-dark.png and b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail-dark.png differ diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail.png b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail.png index 919eaa2118ea..296702c954af 100644 Binary files a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail.png and b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/images/thumbnail.png differ diff --git a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts index daef030b29a4..616619b3e465 100644 --- a/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts @@ -74,6 +74,23 @@ function isPositiveNumber(value: string | number | null | undefined) { ); } +const calculateDifferences = ( + originalValue: number, + comparisonValue: number, +) => { + const valueDifference = originalValue - comparisonValue; + let percentDifferenceNum; + if (!originalValue && !comparisonValue) { + percentDifferenceNum = 0; + } else if (!originalValue || !comparisonValue) { + percentDifferenceNum = originalValue ? 1 : -1; + } else { + percentDifferenceNum = + (originalValue - comparisonValue) / Math.abs(comparisonValue); + } + return { valueDifference, percentDifferenceNum }; +}; + const processComparisonTotals = ( comparisonSuffix: string, totals?: DataRecord[], @@ -149,23 +166,6 @@ const getComparisonColFormatter = ( return formatter; }; -const calculateDifferences = ( - originalValue: number, - comparisonValue: number, -) => { - const valueDifference = originalValue - comparisonValue; - let percentDifferenceNum; - if (!originalValue && !comparisonValue) { - percentDifferenceNum = 0; - } else if (!originalValue || !comparisonValue) { - percentDifferenceNum = originalValue ? 1 : -1; - } else { - percentDifferenceNum = - (originalValue - comparisonValue) / Math.abs(comparisonValue); - } - return { valueDifference, percentDifferenceNum }; -}; - const processComparisonDataRecords = memoizeOne( function processComparisonDataRecords( originalData: DataRecord[] | undefined, @@ -471,6 +471,7 @@ const transformProps = ( filterState, hooks: { setDataMask = () => {} }, emitCrossFilters, + theme, } = chartProps; const { @@ -494,6 +495,36 @@ const transformProps = ( const allowRearrangeColumns = true; + // Calculate time comparison settings early since they're used in multiple places + const isUsingTimeComparison = + !isEmpty(time_compare) && + queryMode === QueryMode.Aggregate && + comparison_type === ComparisonType.Values && + isFeatureEnabled(FeatureFlag.TableV2TimeComparisonEnabled); + + const nonCustomNorInheritShifts = ensureIsArray(formData.time_compare).filter( + (shift: string) => shift !== 'custom' && shift !== 'inherit', + ); + const customOrInheritShifts = ensureIsArray(formData.time_compare).filter( + (shift: string) => shift === 'custom' || shift === 'inherit', + ); + + let timeOffsets: string[] = []; + + if (isUsingTimeComparison && !isEmpty(nonCustomNorInheritShifts)) { + timeOffsets = nonCustomNorInheritShifts; + } + + // Shifts for custom or inherit time comparison + if (isUsingTimeComparison && !isEmpty(customOrInheritShifts)) { + if (customOrInheritShifts.includes('custom')) { + timeOffsets = timeOffsets.concat([formData.start_date_offset]); + } + if (customOrInheritShifts.includes('inherit')) { + timeOffsets = timeOffsets.concat(['inherit']); + } + } + const calculateBasicStyle = ( percentDifferenceNum: number, colorOption: ColorSchemeEnum, @@ -606,12 +637,6 @@ const transformProps = ( : undefined; }; - const isUsingTimeComparison = - !isEmpty(time_compare) && - queryMode === QueryMode.Aggregate && - comparison_type === ComparisonType.Values && - isFeatureEnabled(FeatureFlag.TableV2TimeComparisonEnabled); - let hasServerPageLengthChanged = false; const pageLengthFromMap = serverPageLengthMap.get(slice_id); @@ -624,28 +649,6 @@ const transformProps = ( const timeGrain = extractTimegrain(formData); - const nonCustomNorInheritShifts = ensureIsArray(formData.time_compare).filter( - (shift: string) => shift !== 'custom' && shift !== 'inherit', - ); - const customOrInheritShifts = ensureIsArray(formData.time_compare).filter( - (shift: string) => shift === 'custom' || shift === 'inherit', - ); - - let timeOffsets: string[] = []; - - if (isUsingTimeComparison && !isEmpty(nonCustomNorInheritShifts)) { - timeOffsets = nonCustomNorInheritShifts; - } - - // Shifts for custom or inherit time comparison - if (isUsingTimeComparison && !isEmpty(customOrInheritShifts)) { - if (customOrInheritShifts.includes('custom')) { - timeOffsets = timeOffsets.concat([formData.start_date_offset]); - } - if (customOrInheritShifts.includes('inherit')) { - timeOffsets = timeOffsets.concat(['inherit']); - } - } const comparisonSuffix = isUsingTimeComparison ? ensureIsArray(timeOffsets)[0] : ''; @@ -685,7 +688,7 @@ const transformProps = ( const basicColorFormatters = comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns); const columnColorFormatters = - getColorFormatters(conditionalFormatting, passedData) ?? []; + getColorFormatters(conditionalFormatting, passedData, theme) ?? []; const basicColorColumnFormatters = getBasicColorFormatterForColumn( baseQuery?.data, diff --git a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts index b87e51026461..6181ca23f64b 100644 --- a/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberTotal/transformProps.ts @@ -43,6 +43,7 @@ export default function transformProps( rawFormData, hooks, datasource: { currencyFormats = {}, columnFormats = {} }, + theme, } = chartProps; const { metricNameFontSize, @@ -105,7 +106,7 @@ export default function transformProps( const defaultColorFormatters = [] as ColorFormatters; const colorThresholdFormatters = - getColorFormatters(conditionalFormatting, data, false) ?? + getColorFormatters(conditionalFormatting, data, theme, false) ?? defaultColorFormatters; return { width, diff --git a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts index b7f162f6af7c..9d5339a4dd0c 100644 --- a/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-pivot-table/src/plugin/transformProps.ts @@ -81,6 +81,7 @@ export default function transformProps(chartProps: ChartProps) { filterState, datasource: { verboseMap = {}, columnFormats = {}, currencyFormats = {} }, emitCrossFilters, + theme, } = chartProps; const { data, colnames, coltypes } = queriesData[0]; const { @@ -141,7 +142,11 @@ export default function transformProps(chartProps: ChartProps) { }, {}, ); - const metricColorFormatters = getColorFormatters(conditionalFormatting, data); + const metricColorFormatters = getColorFormatters( + conditionalFormatting, + data, + theme, + ); return { width, diff --git a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts index d83681cc0fe8..ef634e01b86b 100644 --- a/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts +++ b/superset-frontend/plugins/plugin-chart-table/src/transformProps.ts @@ -469,6 +469,7 @@ const transformProps = ( onContextMenu, }, emitCrossFilters, + theme, } = chartProps; const formData = merge( @@ -682,7 +683,7 @@ const transformProps = ( const basicColorFormatters = comparisonColorEnabled && getBasicColorFormatter(baseQuery?.data, columns); const columnColorFormatters = - getColorFormatters(conditionalFormatting, passedData) ?? + getColorFormatters(conditionalFormatting, passedData, theme) ?? defaultColorFormatters; const basicColorColumnFormatters = getBasicColorFormatterForColumn( diff --git a/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx b/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx index 31ef15f731d8..f71d1584d875 100644 --- a/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx +++ b/superset-frontend/src/explore/components/controls/ColumnConfigControl/ColumnConfigItem.tsx @@ -76,7 +76,7 @@ export default memo(function ColumnConfigItem({ alignItems: 'center', position: 'absolute', right: 3 * sizeUnit, - top: 3 * sizeUnit, + top: 4 * sizeUnit, transform: 'translateY(-50%)', gap: sizeUnit, color: theme.colorTextSecondary, diff --git a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx index 7d0fad523910..2c75f249edb9 100644 --- a/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx +++ b/superset-frontend/src/explore/components/controls/ConditionalFormattingControl/FormattingPopoverContent.tsx @@ -17,7 +17,7 @@ * under the License. */ import { useMemo, useState, useEffect } from 'react'; -import { styled, SupersetTheme, t, useTheme } from '@superset-ui/core'; +import { styled, t } from '@superset-ui/core'; import { GenericDataType } from '@apache-superset/core/api/core'; import { Comparator, @@ -56,10 +56,11 @@ const JustifyEnd = styled.div` justify-content: flex-end; `; -const colorSchemeOptions = (theme: SupersetTheme) => [ - { value: theme.colorSuccessBg, label: t('success') }, - { value: theme.colorWarningBg, label: t('alert') }, - { value: theme.colorErrorBg, label: t('error') }, +// Use theme token names instead of hex values to support theme switching +const colorSchemeOptions = () => [ + { value: 'colorSuccessBg', label: t('success') }, + { value: 'colorWarningBg', label: t('alert') }, + { value: 'colorErrorBg', label: t('error') }, ]; const operatorOptions = [ @@ -238,9 +239,8 @@ export const FormattingPopoverContent = ({ columns: { label: string; value: string; dataType: GenericDataType }[]; extraColorChoices?: { label: string; value: string }[]; }) => { - const theme = useTheme(); const [form] = Form.useForm(); - const colorScheme = colorSchemeOptions(theme); + const colorScheme = colorSchemeOptions(); const [showOperatorFields, setShowOperatorFields] = useState( config === undefined || (config?.colorScheme !== ColorSchemeEnum.Green &&