diff --git a/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts b/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts index b07f8539dce..07717f609f8 100644 --- a/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts +++ b/superset-frontend/cypress-base/cypress/e2e/chart_list/list.test.ts @@ -175,7 +175,7 @@ describe('Charts list', () => { interceptDelete(); cy.getBySel('sort-header').contains('Name').click(); - // Modal closes immediatly without this + // Modal closes immediately without this cy.wait(2000); cy.getBySel('table-row').eq(0).contains('3 - Sample chart'); diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/sections/matrixify.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/sections/matrixify.tsx index 0054d5c136c..f70df487fad 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/sections/matrixify.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/sections/matrixify.tsx @@ -20,20 +20,32 @@ import { t } from '@superset-ui/core'; import { ControlPanelSectionConfig } from '../types'; export const matrixifyEnableSection: ControlPanelSectionConfig = { - label: t('Enable matrixify'), + label: t('Matrixify'), expanded: true, controlSetRows: [ [ { - name: 'matrixify_enabled', + name: 'matrixify_enable_horizontal_layout', config: { type: 'CheckboxControl', - label: t('Enable matrixify'), + label: t('Enable horizontal layout (columns)'), + description: t( + 'Create matrix columns by placing charts side-by-side', + ), + default: false, + renderTrigger: true, + }, + }, + ], + [ + { + name: 'matrixify_enable_vertical_layout', + config: { + type: 'CheckboxControl', + label: t('Enable vertical layout (rows)'), + description: t('Create matrix rows by stacking charts vertically'), default: false, renderTrigger: true, - description: t( - 'Transform this chart into a matrix/grid of charts based on dimensions or metrics', - ), }, }, ], @@ -42,9 +54,11 @@ export const matrixifyEnableSection: ControlPanelSectionConfig = { }; export const matrixifySection: ControlPanelSectionConfig = { - label: t('Matrixify'), + label: t('Cell layout & styling'), expanded: false, - visibility: ({ controls }) => controls?.matrixify_enabled?.value === true, + visibility: ({ controls }) => + controls?.matrixify_enable_vertical_layout?.value === true || + controls?.matrixify_enable_horizontal_layout?.value === true, controlSetRows: [ [ { @@ -105,9 +119,10 @@ export const matrixifySection: ControlPanelSectionConfig = { }; export const matrixifyRowSection: ControlPanelSectionConfig = { - label: t('Vertical layout'), + label: t('Vertical layout (rows)'), expanded: false, - visibility: ({ controls }) => controls?.matrixify_enabled?.value === true, + visibility: ({ controls }) => + controls?.matrixify_enable_vertical_layout?.value === true, controlSetRows: [ ['matrixify_show_row_labels'], ['matrixify_mode_rows'], @@ -118,13 +133,14 @@ export const matrixifyRowSection: ControlPanelSectionConfig = { ['matrixify_topn_metric_rows'], ['matrixify_topn_order_rows'], ], - tabOverride: 'data', + tabOverride: 'matrixify', }; export const matrixifyColumnSection: ControlPanelSectionConfig = { - label: t('Horizontal layout'), + label: t('Horizontal layout (columns)'), expanded: false, - visibility: ({ controls }) => controls?.matrixify_enabled?.value === true, + visibility: ({ controls }) => + controls?.matrixify_enable_horizontal_layout?.value === true, controlSetRows: [ ['matrixify_show_column_headers'], ['matrixify_mode_columns'], @@ -135,5 +151,5 @@ export const matrixifyColumnSection: ControlPanelSectionConfig = { ['matrixify_topn_metric_columns'], ['matrixify_topn_order_columns'], ], - tabOverride: 'data', + tabOverride: 'matrixify', }; diff --git a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx index fcc473b5878..39c32e7b8df 100644 --- a/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx +++ b/superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/matrixifyControls.tsx @@ -18,21 +18,49 @@ * under the License. */ -import { t } from '@superset-ui/core'; +import { t, validateNonEmpty } from '@superset-ui/core'; import { SharedControlConfig } from '../types'; import { dndAdhocMetricControl } from './dndControls'; +import { defineSavedMetrics } from '../utils'; /** * Matrixify control definitions * Controls for transforming charts into matrix/grid layouts */ +// Utility function to check if matrixify controls should be visible +const isMatrixifyVisible = ( + controls: any, + axis: 'rows' | 'columns', + mode?: 'metrics' | 'dimensions', + selectionMode?: 'members' | 'topn', +) => { + const layoutControl = `matrixify_enable_${axis === 'rows' ? 'vertical' : 'horizontal'}_layout`; + const modeControl = `matrixify_mode_${axis}`; + const selectionModeControl = `matrixify_dimension_selection_mode_${axis}`; + + const isLayoutEnabled = controls?.[layoutControl]?.value === true; + + if (!isLayoutEnabled) return false; + + if (mode) { + const isModeMatch = controls?.[modeControl]?.value === mode; + if (!isModeMatch) return false; + + if (selectionMode && mode === 'dimensions') { + return controls?.[selectionModeControl]?.value === selectionMode; + } + } + + return true; +}; + // Initialize the controls object that will be populated dynamically const matrixifyControls: Record> = {}; // Dynamically add axis-specific controls (rows and columns) -['columns', 'rows'].forEach(axisParam => { - const axis = axisParam; // Capture the value in a local variable +(['columns', 'rows'] as const).forEach(axisParam => { + const axis: 'rows' | 'columns' = axisParam; matrixifyControls[`matrixify_mode_${axis}`] = { type: 'RadioButtonControl', @@ -43,17 +71,18 @@ const matrixifyControls: Record> = {}; ['dimensions', t('Dimension members')], ], renderTrigger: true, + tabOverride: 'matrixify', + visibility: ({ controls }) => isMatrixifyVisible(controls, axis), }; matrixifyControls[`matrixify_${axis}`] = { ...dndAdhocMetricControl, label: t(`Metrics`), multi: true, - validators: [], // Not required - // description: t(`Select metrics for ${axis}`), + validators: [], // No validation - rely on visibility renderTrigger: true, - visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'metrics', + tabOverride: 'matrixify', + visibility: ({ controls }) => isMatrixifyVisible(controls, axis, 'metrics'), }; // Combined dimension and values control @@ -62,8 +91,9 @@ const matrixifyControls: Record> = {}; label: t(`Dimension selection`), description: t(`Select dimension and values`), default: { dimension: '', values: [] }, - validators: [], // Not required + validators: [], // No validation - rely on visibility renderTrigger: true, + tabOverride: 'matrixify', shouldMapStateToProps: (prevState, state) => { // Recalculate when any relevant form_data field changes const fieldsToCheck = [ @@ -82,24 +112,40 @@ const matrixifyControls: Record> = {}; const getValue = (key: string, defaultValue?: any) => form_data?.[key] ?? controls?.[key]?.value ?? defaultValue; + const selectionMode = getValue( + `matrixify_dimension_selection_mode_${axis}`, + 'members', + ); + + const isVisible = isMatrixifyVisible(controls, axis, 'dimensions'); + + // Validate dimension is selected when visible + const dimensionValidator = (value: any) => { + if (!value?.dimension) { + return t('Dimension is required'); + } + return false; + }; + + // Additional validation for topN mode + const validators = isVisible + ? [dimensionValidator, validateNonEmpty] + : []; + return { datasource, - selectionMode: getValue( - `matrixify_dimension_selection_mode_${axis}`, - 'members', - ), + selectionMode, topNMetric: getValue(`matrixify_topn_metric_${axis}`), topNValue: getValue(`matrixify_topn_value_${axis}`), topNOrder: getValue(`matrixify_topn_order_${axis}`), formData: form_data, + validators, }; }, visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions', + isMatrixifyVisible(controls, axis, 'dimensions'), }; - // Dimension picker for TopN mode (just dimension, no values) - // NOTE: This is now handled by matrixify_dimension control, so hiding it matrixifyControls[`matrixify_topn_dimension_${axis}`] = { type: 'SelectControl', label: t('Dimension'), @@ -127,33 +173,67 @@ const matrixifyControls: Record> = {}; ['topn', t('Top n')], ], renderTrigger: true, + tabOverride: 'matrixify', visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions', + isMatrixifyVisible(controls, axis, 'dimensions'), }; // TopN controls matrixifyControls[`matrixify_topn_value_${axis}`] = { - type: 'TextControl', + type: 'NumberControl', label: t(`Number of top values`), description: t(`How many top values to select`), default: 10, isInt: true, + validators: [], + renderTrigger: true, + tabOverride: 'matrixify', visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions' && - controls?.[`matrixify_dimension_selection_mode_${axis}`]?.value === + isMatrixifyVisible(controls, axis, 'dimensions', 'topn'), + mapStateToProps: ({ controls }) => { + const isVisible = isMatrixifyVisible( + controls, + axis, + 'dimensions', 'topn', + ); + + return { + validators: isVisible ? [validateNonEmpty] : [], + }; + }, }; matrixifyControls[`matrixify_topn_metric_${axis}`] = { ...dndAdhocMetricControl, label: t(`Metric for ordering`), multi: false, - validators: [], // Not required + validators: [], description: t(`Metric to use for ordering Top N values`), + tabOverride: 'matrixify', visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions' && - controls?.[`matrixify_dimension_selection_mode_${axis}`]?.value === + isMatrixifyVisible(controls, axis, 'dimensions', 'topn'), + mapStateToProps: (state, controlState) => { + const { controls, datasource } = state; + const isVisible = isMatrixifyVisible( + controls, + axis, + 'dimensions', 'topn', + ); + + const originalProps = + dndAdhocMetricControl.mapStateToProps?.(state, controlState) || {}; + + return { + ...originalProps, + columns: datasource?.columns || [], + savedMetrics: defineSavedMetrics(datasource), + datasource, + datasourceType: datasource?.type, + validators: isVisible ? [validateNonEmpty] : [], + }; + }, }; matrixifyControls[`matrixify_topn_order_${axis}`] = { @@ -164,10 +244,10 @@ const matrixifyControls: Record> = {}; ['asc', t('Ascending')], ['desc', t('Descending')], ], + renderTrigger: true, + tabOverride: 'matrixify', visibility: ({ controls }) => - controls?.[`matrixify_mode_${axis}`]?.value === 'dimensions' && - controls?.[`matrixify_dimension_selection_mode_${axis}`]?.value === - 'topn', + isMatrixifyVisible(controls, axis, 'dimensions', 'topn'), }; }); @@ -213,15 +293,22 @@ matrixifyControls.matrixify_charts_per_row = { !controls?.matrixify_fit_columns_dynamically?.value, }; -// Main enable control -matrixifyControls.matrixify_enabled = { +matrixifyControls.matrixify_enable_vertical_layout = { type: 'CheckboxControl', - label: t('Enable matrixify'), - description: t( - 'Transform this chart into a matrix/grid of charts based on dimensions or metrics', - ), + label: t('Enable vertical layout (rows)'), + description: t('Create matrix rows by stacking charts vertically'), default: false, renderTrigger: true, + tabOverride: 'matrixify', +}; + +matrixifyControls.matrixify_enable_horizontal_layout = { + type: 'CheckboxControl', + label: t('Enable horizontal layout (columns)'), + description: t('Create matrix columns by placing charts side-by-side'), + default: false, + renderTrigger: true, + tabOverride: 'matrixify', }; // Cell title control for Matrixify @@ -234,8 +321,8 @@ matrixifyControls.matrixify_cell_title_template = { default: '', renderTrigger: true, visibility: ({ controls }) => - (controls?.matrixify_mode_rows?.value || - controls?.matrixify_mode_columns?.value) !== undefined, + controls?.matrixify_enable_vertical_layout?.value === true || + controls?.matrixify_enable_horizontal_layout?.value === true, }; // Matrix display controls @@ -245,9 +332,9 @@ matrixifyControls.matrixify_show_row_labels = { description: t('Display labels for each row on the left side of the matrix'), default: true, renderTrigger: true, + tabOverride: 'matrixify', visibility: ({ controls }) => - (controls?.matrixify_mode_rows?.value || - controls?.matrixify_mode_columns?.value) !== undefined, + controls?.matrixify_enable_vertical_layout?.value === true, }; matrixifyControls.matrixify_show_column_headers = { @@ -256,9 +343,9 @@ matrixifyControls.matrixify_show_column_headers = { description: t('Display headers for each column at the top of the matrix'), default: true, renderTrigger: true, + tabOverride: 'matrixify', visibility: ({ controls }) => - (controls?.matrixify_mode_rows?.value || - controls?.matrixify_mode_columns?.value) !== undefined, + controls?.matrixify_enable_horizontal_layout?.value === true, }; export { matrixifyControls }; diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.ts b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.ts index afc075ca19a..7895bf23a25 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridGenerator.ts @@ -276,10 +276,10 @@ export function generateMatrixifyGrid( const cellFormData = generateCellFormData( formData, - rowCount > 1 ? config.rows : null, - colCount > 1 ? config.columns : null, - rowCount > 1 ? row : null, - colCount > 1 ? col : null, + rowCount > 0 ? config.rows : null, + colCount > 0 ? config.columns : null, + rowCount > 0 ? row : null, + colCount > 0 ? col : null, ); // Generate title using template if provided diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx index 7dd05e06c67..d56bf8f6926 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.test.tsx @@ -74,7 +74,8 @@ test('should create single group when fitting columns dynamically', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: true, matrixify_charts_per_row: 3, matrixify_show_row_labels: true, @@ -123,7 +124,8 @@ test('should create multiple groups when not fitting columns dynamically', () => const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: false, matrixify_charts_per_row: 3, matrixify_show_row_labels: true, @@ -158,7 +160,8 @@ test('should handle exact division of columns', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: false, matrixify_charts_per_row: 2, matrixify_show_row_labels: true, @@ -186,7 +189,8 @@ test('should handle case where charts_per_row exceeds total columns', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: false, matrixify_charts_per_row: 5, matrixify_show_row_labels: true, @@ -216,7 +220,8 @@ test('should show headers for each group when wrapping occurs', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: false, matrixify_charts_per_row: 2, matrixify_show_row_labels: true, @@ -250,7 +255,8 @@ test('should show headers only on first row when not wrapping', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: true, // No wrapping matrixify_show_row_labels: true, matrixify_show_column_headers: true, @@ -279,7 +285,8 @@ test('should hide headers when disabled', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_show_row_labels: false, matrixify_show_column_headers: false, }; @@ -306,7 +313,8 @@ test('should place cells correctly in wrapped layout', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, matrixify_fit_columns_dynamically: false, matrixify_charts_per_row: 2, matrixify_show_row_labels: true, @@ -336,7 +344,8 @@ test('should handle null grid gracefully', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, }; const { container } = renderWithTheme( @@ -357,7 +366,8 @@ test('should handle empty grid gracefully', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, }; const { container } = renderWithTheme( @@ -381,7 +391,8 @@ test('should use default values for missing configuration', () => { const formData = { viz_type: 'test_chart', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, + matrixify_enable_horizontal_layout: true, // Missing optional configurations }; diff --git a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx index f06d3b949be..58939477ee1 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx +++ b/superset-frontend/packages/superset-ui-core/src/chart/components/Matrixify/MatrixifyGridRenderer.tsx @@ -128,9 +128,13 @@ function MatrixifyGridRenderer({ [formData], ); - // Determine layout parameters - const showRowLabels = formData.matrixify_show_row_labels ?? true; - const showColumnHeaders = formData.matrixify_show_column_headers ?? true; + // Determine layout parameters - only show headers/labels if layout is enabled + const showRowLabels = + formData.matrixify_enable_vertical_layout === true && + (formData.matrixify_show_row_labels ?? true); + const showColumnHeaders = + formData.matrixify_enable_horizontal_layout === true && + (formData.matrixify_show_column_headers ?? true); const rowHeight = formData.matrixify_row_height || DEFAULT_ROW_HEIGHT; const fitColumnsDynamically = formData.matrixify_fit_columns_dynamically ?? true; diff --git a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts index bb22958028e..cdc556b69fb 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.test.ts @@ -37,10 +37,11 @@ test('isMatrixifyEnabled should return false when no matrixify configuration exi expect(isMatrixifyEnabled(formData)).toBe(false); }); -test('isMatrixifyEnabled should return false when matrixify_enabled is false', () => { +test('isMatrixifyEnabled should return false when layout controls are false', () => { const formData = { viz_type: 'table', - matrixify_enabled: false, + matrixify_enable_vertical_layout: false, + matrixify_enable_horizontal_layout: false, matrixify_mode_rows: 'metrics', matrixify_rows: [createMetric('Revenue')], } as MatrixifyFormData; @@ -51,7 +52,7 @@ test('isMatrixifyEnabled should return false when matrixify_enabled is false', ( test('isMatrixifyEnabled should return true for valid metrics mode configuration', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_mode_columns: 'metrics', matrixify_rows: [createMetric('Revenue')], @@ -64,7 +65,7 @@ test('isMatrixifyEnabled should return true for valid metrics mode configuration test('isMatrixifyEnabled should return true for valid dimensions mode configuration', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'dimensions', matrixify_mode_columns: 'dimensions', matrixify_dimension_rows: { dimension: 'country', values: ['USA'] }, @@ -77,7 +78,7 @@ test('isMatrixifyEnabled should return true for valid dimensions mode configurat test('isMatrixifyEnabled should return true for mixed mode configuration', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_mode_columns: 'dimensions', matrixify_rows: [createMetric('Revenue')], @@ -90,7 +91,7 @@ test('isMatrixifyEnabled should return true for mixed mode configuration', () => test('isMatrixifyEnabled should return true for topn dimension selection mode', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'dimensions', matrixify_mode_columns: 'dimensions', matrixify_dimension_rows: { @@ -109,7 +110,7 @@ test('isMatrixifyEnabled should return true for topn dimension selection mode', test('isMatrixifyEnabled should return false when both axes have empty metrics arrays', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_mode_columns: 'metrics', matrixify_rows: [], @@ -122,7 +123,7 @@ test('isMatrixifyEnabled should return false when both axes have empty metrics a test('isMatrixifyEnabled should return false when both dimensions have empty values and no topn mode', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'dimensions', matrixify_mode_columns: 'dimensions', matrixify_dimension_rows: { dimension: 'country', values: [] }, @@ -140,7 +141,7 @@ test('getMatrixifyConfig should return null when no matrixify configuration exis test('getMatrixifyConfig should return valid config for metrics mode', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_mode_columns: 'metrics', matrixify_rows: [createMetric('Revenue')], @@ -158,7 +159,7 @@ test('getMatrixifyConfig should return valid config for metrics mode', () => { test('getMatrixifyConfig should return valid config for dimensions mode', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'dimensions', matrixify_mode_columns: 'dimensions', matrixify_dimension_rows: { dimension: 'country', values: ['USA'] }, @@ -182,7 +183,7 @@ test('getMatrixifyConfig should return valid config for dimensions mode', () => test('getMatrixifyConfig should handle topn selection mode', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'dimensions', matrixify_mode_columns: 'dimensions', matrixify_dimension_rows: { @@ -212,7 +213,7 @@ test('getMatrixifyValidationErrors should return empty array when matrixify is n test('getMatrixifyValidationErrors should return empty array when properly configured', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_mode_columns: 'metrics', matrixify_rows: [createMetric('Revenue')], @@ -225,7 +226,7 @@ test('getMatrixifyValidationErrors should return empty array when properly confi test('getMatrixifyValidationErrors should return error when enabled but no configuration exists', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, } as MatrixifyFormData; const errors = getMatrixifyValidationErrors(formData); @@ -235,7 +236,7 @@ test('getMatrixifyValidationErrors should return error when enabled but no confi test('getMatrixifyValidationErrors should return error when metrics mode has no metrics', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_rows: [], matrixify_columns: [], @@ -261,7 +262,7 @@ test('should handle empty form data object', () => { test('should handle partial configuration with one axis only', () => { const formData = { viz_type: 'table', - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_mode_rows: 'metrics', matrixify_rows: [createMetric('Revenue')], // No columns configuration diff --git a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts index b3e8f8397b0..6573cf7afa4 100644 --- a/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts +++ b/superset-frontend/packages/superset-ui-core/src/chart/types/matrixify.ts @@ -99,6 +99,10 @@ export interface MatrixifyFormData { // Enable/disable matrixify functionality matrixify_enabled?: boolean; + // Layout enable controls + matrixify_enable_vertical_layout?: boolean; + matrixify_enable_horizontal_layout?: boolean; + // Row axis configuration matrixify_mode_rows?: MatrixifyMode; matrixify_rows?: AdhocMetric[]; @@ -177,8 +181,12 @@ export function getMatrixifyConfig( * Check if Matrixify is enabled and properly configured */ export function isMatrixifyEnabled(formData: MatrixifyFormData): boolean { - // First check if matrixify is explicitly enabled via checkbox - if (!formData.matrixify_enabled) { + // Check if either vertical or horizontal layout is enabled + const hasVerticalLayout = formData.matrixify_enable_vertical_layout === true; + const hasHorizontalLayout = + formData.matrixify_enable_horizontal_layout === true; + + if (!hasVerticalLayout && !hasHorizontalLayout) { return false; } @@ -216,7 +224,11 @@ export function getMatrixifyValidationErrors( const errors: string[] = []; // Only validate if matrixify is enabled - if (!formData.matrixify_enabled) { + const hasVerticalLayout = formData.matrixify_enable_vertical_layout === true; + const hasHorizontalLayout = + formData.matrixify_enable_horizontal_layout === true; + + if (!hasVerticalLayout && !hasHorizontalLayout) { return errors; } diff --git a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx index 457636c1090..98e7fb8a894 100644 --- a/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx +++ b/superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx @@ -188,7 +188,10 @@ const ChartContextMenu = ( isFeatureEnabled(FeatureFlag.DrillBy) && canDrillBy && isDisplayed(ContextMenuItem.DrillBy) && - !formData.matrixify_enabled; // Disable drill by when matrixify is enabled + !( + formData.matrixify_enable_vertical_layout === true || + formData.matrixify_enable_horizontal_layout === true + ); // Disable drill by when matrixify is enabled const datasetResource = useDatasetDrillInfo( formData.datasource, diff --git a/superset-frontend/src/components/Chart/ChartRenderer.jsx b/superset-frontend/src/components/Chart/ChartRenderer.jsx index d10d7b0439a..410a5888132 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.jsx @@ -153,7 +153,10 @@ class ChartRenderer extends Component { // Check if any matrixify-related properties have changed const hasMatrixifyChanges = () => { - if (!nextProps.formData.matrixify_enabled) return false; + const isMatrixifyEnabled = + nextProps.formData.matrixify_enable_vertical_layout === true || + nextProps.formData.matrixify_enable_horizontal_layout === true; + if (!isMatrixifyEnabled) return false; // Check all matrixify-related properties const matrixifyKeys = Object.keys(nextProps.formData).filter(key => diff --git a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx index c5734fdcce0..c6cda719a74 100644 --- a/superset-frontend/src/components/Chart/ChartRenderer.test.jsx +++ b/superset-frontend/src/components/Chart/ChartRenderer.test.jsx @@ -99,7 +99,7 @@ test('should detect changes in matrixify properties', () => { ...requiredProps, formData: { ...requiredProps.formData, - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA'] }, matrixify_dimension_y: { dimension: 'category', values: ['Tech'] }, matrixify_charts_per_row: 3, @@ -114,7 +114,7 @@ test('should detect changes in matrixify properties', () => { // Since we can't directly test shouldComponentUpdate, we verify the component // correctly identifies matrixify-related properties by checking the implementation - expect(initialProps.formData.matrixify_enabled).toBe(true); + expect(initialProps.formData.matrixify_enable_vertical_layout).toBe(true); expect(initialProps.formData.matrixify_dimension_x).toEqual({ dimension: 'country', values: ['USA'], @@ -143,7 +143,7 @@ test('should identify matrixify property changes correctly', () => { const initialProps = { ...requiredProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA'] }, matrixify_charts_per_row: 3, }, @@ -161,7 +161,7 @@ test('should identify matrixify property changes correctly', () => { const updatedProps = { ...initialProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA', 'Canada'], // Changed @@ -199,7 +199,7 @@ test('should handle matrixify-related form data changes', () => { const updatedProps = { ...initialProps, formData: { - matrixify_enabled: true, // This is a significant change + matrixify_enable_vertical_layout: true, // This is a significant change regular_control: 'value1', }, }; @@ -216,7 +216,7 @@ test('should detect matrixify property addition', () => { const initialProps = { ...requiredProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, // No matrixify_dimension_x initially }, queriesResponse: [{ data: 'current' }], @@ -233,7 +233,7 @@ test('should detect matrixify property addition', () => { const updatedProps = { ...initialProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA'] }, // Added }, }; @@ -250,7 +250,7 @@ test('should detect nested matrixify property changes', () => { const initialProps = { ...requiredProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA'], @@ -271,7 +271,7 @@ test('should detect nested matrixify property changes', () => { const updatedProps = { ...initialProps, formData: { - matrixify_enabled: true, + matrixify_enable_vertical_layout: true, matrixify_dimension_x: { dimension: 'country', values: ['USA'], diff --git a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx index 3ac4a6dd8fc..85b6af61ec7 100644 --- a/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx +++ b/superset-frontend/src/components/Chart/DrillBy/DrillByMenuItems.tsx @@ -201,7 +201,10 @@ export const DrillByMenuItems = ({ }; // Don't render drill by menu items when matrixify is enabled - if (formData.matrixify_enabled) { + if ( + formData.matrixify_enable_vertical_layout === true || + formData.matrixify_enable_horizontal_layout === true + ) { return null; } diff --git a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx index b39f433afcb..9e30cef4904 100644 --- a/superset-frontend/src/explore/components/ControlPanelsContainer.tsx +++ b/superset-frontend/src/explore/components/ControlPanelsContainer.tsx @@ -96,6 +96,7 @@ export type ControlPanelsContainerProps = { form_data: QueryFormData; isDatasourceMetaLoading: boolean; errorMessage: ReactNode; + buttonErrorMessage?: ReactNode; // Error message for RunQueryButton (includes all errors) onQuery: () => void; onStop: () => void; canStopQuery: boolean; @@ -766,37 +767,89 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => { props.errorMessage, ]); + const showCustomizeTab = customizeSections.length > 0; + const showMatrixifyTab = isFeatureEnabled(FeatureFlag.Matrixify); + + // Check if matrixify sections have validation errors + const matrixifyHasErrors = useMemo(() => { + if (!showMatrixifyTab) return false; + + return matrixifySections.some(section => + section.controlSetRows.some(rows => + rows.some(item => { + const controlName = + typeof item === 'string' + ? item + : item && 'name' in item + ? item.name + : null; + return ( + controlName && + controlName in props.controls && + props.controls[controlName].validationErrors && + props.controls[controlName].validationErrors.length > 0 + ); + }), + ), + ); + }, [showMatrixifyTab, matrixifySections, props.controls]); + + // Create Matrixify tab label with Beta tag and validation errors + const matrixifyTabLabel = useMemo( + () => ( + <> + {t('Matrixify')} + {matrixifyHasErrors && ( + css` + margin-left: ${theme.sizeUnit * 2}px; + `} + > + {' '} + + + + + )}{' '} + + + + + ), + [ + matrixifyHasErrors, + theme.colorErrorText, + theme.sizeUnit, + theme.fontSizeSM, + ], + ); + const controlPanelRegistry = getChartControlPanelRegistry(); if (!controlPanelRegistry.has(form_data.viz_type) && pluginContext.loading) { return ; } - const showCustomizeTab = customizeSections.length > 0; - const showMatrixifyTab = isFeatureEnabled(FeatureFlag.Matrixify); - - // Create Matrixify tab label with Beta tag - const matrixifyTabLabel = ( - <> - {t('Matrixify')}{' '} - - - - - ); - return ( { {renderChart()} @@ -412,7 +415,8 @@ const ExploreChartPanel = ({ chart.chartUpdateEndTime, refreshCachedQuery, formData?.row_limit, - formData?.matrixify_enabled, + formData?.matrixify_enable_vertical_layout, + formData?.matrixify_enable_horizontal_layout, renderChart, ], ); diff --git a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx index 1fd22603de4..c65f14916fc 100644 --- a/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx +++ b/superset-frontend/src/explore/components/ExploreViewContainer/index.jsx @@ -640,6 +640,7 @@ function ExploreViewContainer(props) { } const errorMessage = useMemo(() => { + // Include all controls with validation errors (for button disabling) const controlsWithErrors = Object.values(props.controls).filter( control => control.validationErrors && control.validationErrors.length > 0, @@ -679,11 +680,62 @@ function ExploreViewContainer(props) { return errorMessage; }, [props.controls]); + // Error message for Data tab only (excludes matrixify controls) + const dataTabErrorMessage = useMemo(() => { + const controlsWithErrors = Object.values(props.controls).filter( + control => + control.validationErrors && + control.validationErrors.length > 0 && + control.tabOverride !== 'matrixify', // Exclude matrixify controls from Data tab + ); + if (controlsWithErrors.length === 0) { + return null; + } + + const errorMessages = controlsWithErrors.map( + control => control.validationErrors, + ); + const uniqueErrorMessages = [...new Set(errorMessages.flat())]; + + const errors = uniqueErrorMessages + .map(message => { + const matchingLabels = controlsWithErrors + .filter(control => control.validationErrors?.includes(message)) + .map(control => + typeof control.label === 'function' + ? control.label(props.exploreState) + : control.label, + ); + return [matchingLabels, message]; + }) + .map(([labels, message]) => ( +
+ {labels.length > 1 ? t('Controls labeled ') : t('Control labeled ')} + {` ${labels.join(', ')}`} + : {message} +
+ )); + + let dataTabErrorMessage; + if (errors.length > 0) { + dataTabErrorMessage = ( +
+ {errors} +
+ ); + } + return dataTabErrorMessage; + }, [props.controls]); + function renderChartContainer() { return ( @@ -829,7 +881,8 @@ function ExploreViewContainer(props) { onQuery={onQuery} onStop={onStop} canStopQuery={props.can_add || props.can_overwrite} - errorMessage={errorMessage} + errorMessage={dataTabErrorMessage} + buttonErrorMessage={errorMessage} chartIsStale={chartIsStale} /> diff --git a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx index 1ca08cf2e9d..0acf0fd89ff 100644 --- a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.test.tsx @@ -274,30 +274,6 @@ test('should fetch TopN values when all params are provided', async () => { }); }); -test('should show loading state while fetching TopN values', () => { - const mockFetchTopNValues = fetchTopNValues as jest.MockedFunction< - typeof fetchTopNValues - >; - const value: MatrixifyDimensionControlValue = { - dimension: 'country', - values: [], - }; - - mockFetchTopNValues.mockImplementation(() => new Promise(() => {})); // Never resolves - - render( - , - ); - - expect(screen.getByText('Loading top values...')).toBeInTheDocument(); -}); - test('should display error when TopN fetch fails', async () => { const mockFetchTopNValues = fetchTopNValues as jest.MockedFunction< typeof fetchTopNValues diff --git a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx index 3623117d835..72e27b6b9b4 100644 --- a/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx +++ b/superset-frontend/src/explore/components/controls/MatrixifyDimensionControl.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { useEffect, useState } from 'react'; +import { useEffect, useState, useRef, useMemo } from 'react'; import { t, SupersetClient, getColumnLabel } from '@superset-ui/core'; import { Select, Space } from '@superset-ui/core/components'; import ControlHeader from 'src/explore/components/ControlHeader'; @@ -40,11 +40,13 @@ interface MatrixifyDimensionControlProps { label?: string; description?: string; hovered?: boolean; + renderTrigger?: boolean; selectionMode?: 'members' | 'topn'; topNMetric?: string; topNValue?: number; topNOrder?: 'ASC' | 'DESC'; formData?: any; // For access to filters and time range + validationErrors?: string[]; } export default function MatrixifyDimensionControl( @@ -56,12 +58,13 @@ export default function MatrixifyDimensionControl( onChange, label, description, - hovered, + renderTrigger, selectionMode = 'members', topNMetric, topNValue, topNOrder = 'DESC', formData, + validationErrors, } = props; const [dimensionOptions, setDimensionOptions] = useState< @@ -71,8 +74,24 @@ export default function MatrixifyDimensionControl( Array<{ label: string; value: any }> >([]); const [loadingValues, setLoadingValues] = useState(false); - const [loadingTopN, setLoadingTopN] = useState(false); const [topNError, setTopNError] = useState(null); + const [dimensionHovered, setDimensionHovered] = useState(false); + const [valuesHovered, setValuesHovered] = useState(false); + const prevSelectionMode = useRef(selectionMode); + + // Reset values when selection mode changes + useEffect(() => { + if (prevSelectionMode.current !== selectionMode) { + prevSelectionMode.current = selectionMode; + if (value?.values && value.values.length > 0) { + onChange({ + dimension: value.dimension, + values: [], + topNValues: [], + }); + } + } + }, [selectionMode, value]); // Initialize dimension options from datasource useEffect(() => { @@ -140,19 +159,22 @@ export default function MatrixifyDimensionControl( }, [value?.dimension, datasource, selectionMode]); // Convert topNValue to number for consistent comparison - const topNValueNum = - typeof topNValue === 'string' ? parseInt(topNValue, 10) : topNValue; + const topNValueNum = useMemo(() => { + if (topNValue === null || topNValue === undefined) { + return null; + } + if (typeof topNValue === 'string') { + if (topNValue === '') return null; + const num = parseInt(topNValue, 10); + return Number.isNaN(num) ? null : num; + } + return typeof topNValue === 'number' ? topNValue : null; + }, [topNValue]); // Load TopN values when in TopN mode useEffect(() => { - if ( - !value?.dimension || - !datasource || - selectionMode !== 'topn' || - !topNMetric || - !topNValueNum - ) { - // Clear the values when not in topn mode + if (!value?.dimension || !datasource || selectionMode !== 'topn') { + // Clear values when switching away from topn mode if ( selectionMode !== 'topn' && value?.values && @@ -167,11 +189,15 @@ export default function MatrixifyDimensionControl( return undefined; } + // If we don't have the required topN parameters, just return without loading + if (!topNMetric || !topNValueNum || topNValueNum <= 0) { + return undefined; + } + const controller = new AbortController(); const { signal } = controller; const loadTopNValues = async () => { - setLoadingTopN(true); setTopNError(null); try { @@ -205,10 +231,6 @@ export default function MatrixifyDimensionControl( topNValues: [], }); } - } finally { - if (!signal.aborted) { - setLoadingTopN(false); - } } }; @@ -222,11 +244,10 @@ export default function MatrixifyDimensionControl( datasource, selectionMode, topNMetric, - topNValueNum, // Use the converted number + topNValueNum, // Use the converted/validated number topNOrder, formData?.adhoc_filters, formData?.time_range, - onChange, // Add onChange to deps ]); const handleDimensionChange = (dimension: string) => { @@ -246,49 +267,60 @@ export default function MatrixifyDimensionControl( return ( - } - mode="multiple" - onChange={handleValuesChange} - options={valueOptions} - placeholder={t('Select values')} - loading={loadingValues} + onChange={handleDimensionChange} + options={dimensionOptions.map(([val, label]) => ({ + value: val, + label, + }))} + placeholder={t('Select a dimension')} allowClear - showSearch - notFoundContent={t('No results')} /> + + + {value?.dimension && selectionMode === 'members' && ( +
setValuesHovered(true)} + onMouseLeave={() => setValuesHovered(false)} + > +