From 30e3a76ed444ec2b86cc0da67ab1291b3b829f81 Mon Sep 17 00:00:00 2001 From: Anan Zhuang Date: Mon, 29 Apr 2024 16:12:47 +0000 Subject: [PATCH] [VisBuilder][BUG] Flat render structure in Metric and Table Vis This issue is caused by the callback behavior in ReactExpressionRenderer. The callback to ReactExpressionRenderer to update isloading state is lost if we wrap the render vis with VisualizationContainer. This PR moved VisualizationContainer directly in MetricVisComponent. When put VisualizationContainer directly in the MetricVisComponent, all the lifecycle methods and hooks within MetricVisComponent directly influence the rendering of VisualizationContainer. This means that calls to renderComplete and other lifecycle integrations are more directly managed. Issue Resolved: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/6671 Signed-off-by: Anan Zhuang --- .../metric_vis_component.test.tsx.snap | 42 +++++++----- .../components/metric_vis_component.tsx | 13 ++-- .../public/metric_vis_renderer.tsx | 20 +++--- .../public/components/table_vis_app.test.tsx | 67 ++++++++++++++----- .../public/components/table_vis_app.tsx | 48 +++++++------ .../public/table_vis_renderer.tsx | 8 +-- .../public/utils/get_table_ui_state.ts | 4 +- 7 files changed, 121 insertions(+), 81 deletions(-) diff --git a/src/plugins/vis_type_metric/public/components/__snapshots__/metric_vis_component.test.tsx.snap b/src/plugins/vis_type_metric/public/components/__snapshots__/metric_vis_component.test.tsx.snap index f07fdfa682d..27c9507a549 100644 --- a/src/plugins/vis_type_metric/public/components/__snapshots__/metric_vis_component.test.tsx.snap +++ b/src/plugins/vis_type_metric/public/components/__snapshots__/metric_vis_component.test.tsx.snap @@ -1,7 +1,10 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`MetricVisComponent should render correct structure for multi-value metrics 1`] = ` -Array [ + , + /> , -] + /> + `; exports[`MetricVisComponent should render correct structure for single metric 1`] = ` - + + showLabel={true} + /> + `; diff --git a/src/plugins/vis_type_metric/public/components/metric_vis_component.tsx b/src/plugins/vis_type_metric/public/components/metric_vis_component.tsx index 8162eb7d197..42e46054b9d 100644 --- a/src/plugins/vis_type_metric/public/components/metric_vis_component.tsx +++ b/src/plugins/vis_type_metric/public/components/metric_vis_component.tsx @@ -40,6 +40,7 @@ import { VisParams, MetricVisMetric } from '../types'; import { getFormatService } from '../services'; import { SchemaConfig } from '../../../visualizations/public'; import { Range } from '../../../expressions/public'; +import { VisualizationContainer } from '../../../visualizations/public'; import './metric_vis.scss'; @@ -214,12 +215,12 @@ class MetricVisComponent extends Component { } render() { - let metricsHtml; - if (this.props.visData) { - const metrics = this.processTableGroups(this.props.visData); - metricsHtml = metrics.map(this.renderMetric); - } - return metricsHtml; + const metrics = this.props.visData.rows ? this.processTableGroups(this.props.visData) : []; + return ( + + {metrics.length > 0 ? metrics.map(this.renderMetric) : null} + + ); } } diff --git a/src/plugins/vis_type_metric/public/metric_vis_renderer.tsx b/src/plugins/vis_type_metric/public/metric_vis_renderer.tsx index dcd3fcd0a72..14b0c28f2c5 100644 --- a/src/plugins/vis_type_metric/public/metric_vis_renderer.tsx +++ b/src/plugins/vis_type_metric/public/metric_vis_renderer.tsx @@ -28,14 +28,14 @@ * under the License. */ -import React, { lazy } from 'react'; +import React from 'react'; import { render, unmountComponentAtNode } from 'react-dom'; -import { VisualizationContainer } from '../../visualizations/public'; import { ExpressionRenderDefinition } from '../../expressions/common/expression_renderers'; import { MetricVisRenderValue } from './metric_vis_fn'; +import MetricVisComponent from './components/metric_vis_component'; + // @ts-ignore -const MetricVisComponent = lazy(() => import('./components/metric_vis_component')); export const metricVisRenderer: () => ExpressionRenderDefinition = () => ({ name: 'metric_vis', @@ -47,14 +47,12 @@ export const metricVisRenderer: () => ExpressionRenderDefinition - - , + , domNode ); }, diff --git a/src/plugins/vis_type_table/public/components/table_vis_app.test.tsx b/src/plugins/vis_type_table/public/components/table_vis_app.test.tsx index f2a347dcd0a..58e38a34636 100644 --- a/src/plugins/vis_type_table/public/components/table_vis_app.test.tsx +++ b/src/plugins/vis_type_table/public/components/table_vis_app.test.tsx @@ -9,7 +9,7 @@ import { coreMock } from '../../../../core/public/mocks'; import { IInterpreterRenderHandlers } from 'src/plugins/expressions'; import { TableVisApp } from './table_vis_app'; import { TableVisConfig } from '../types'; -import { TableVisData } from '../table_vis_response_handler'; +import { TableVisData, FormattedTableContext } from '../table_vis_response_handler'; jest.mock('./table_vis_component_group', () => ({ TableVisComponentGroup: () => ( @@ -42,15 +42,24 @@ describe('TableVisApp', () => { } as unknown) as IInterpreterRenderHandlers; const visConfigMock = ({} as unknown) as TableVisConfig; - it('should render TableVisComponent if no split table', () => { - const visDataMock = { - table: { - columns: [], - rows: [], - formattedColumns: [], + const createMockFormattedTableContext = (rowCount: number): FormattedTableContext => ({ + columns: [{ id: 'col1', name: 'Column 1' }], + rows: Array(rowCount).fill({ col1: 'value' }), + formattedColumns: [ + { + id: 'col1', + title: 'Column 1', + formatter: {} as any, + filterable: true, }, + ], + }); + + it('should render TableVisComponent if no split table and has rows', () => { + const visDataMock: TableVisData = { + table: createMockFormattedTableContext(1), tableGroups: [], - } as TableVisData; + }; const { getByTestId } = render( { }); it('should render TableVisComponentGroup component if split direction is column', () => { - const visDataMock = { - tableGroups: [], + const visDataMock: TableVisData = { + tableGroups: [ + { + table: createMockFormattedTableContext(1), + title: 'Group 1', + }, + ], direction: 'column', - } as TableVisData; + }; const { container, getByTestId } = render( { handlers={handlersMock} /> ); - expect(container.outerHTML.includes('visTable')).toBe(true); + expect(container.querySelector('.visTable')).not.toBeNull(); expect(getByTestId('TableVisComponentGroup')).toBeInTheDocument(); }); it('should render TableVisComponentGroup component if split direction is row', () => { - const visDataMock = { - tableGroups: [], + const visDataMock: TableVisData = { + tableGroups: [ + { + table: createMockFormattedTableContext(1), + title: 'Group 1', + }, + ], direction: 'row', - } as TableVisData; + }; const { container, getByTestId } = render( { handlers={handlersMock} /> ); - expect(container.outerHTML.includes('visTable')).toBe(true); + expect(container.querySelector('.visTable')).not.toBeNull(); expect(getByTestId('TableVisComponentGroup')).toBeInTheDocument(); }); + + it('should render "No results found" when there are no rows', () => { + const visDataMock: TableVisData = { + table: createMockFormattedTableContext(0), + tableGroups: [], + }; + const { getByText } = render( + + ); + expect(getByText('No results found')).toBeInTheDocument(); + }); }); diff --git a/src/plugins/vis_type_table/public/components/table_vis_app.tsx b/src/plugins/vis_type_table/public/components/table_vis_app.tsx index cbde913452d..ccdab8cdaf8 100644 --- a/src/plugins/vis_type_table/public/components/table_vis_app.tsx +++ b/src/plugins/vis_type_table/public/components/table_vis_app.tsx @@ -16,6 +16,7 @@ import { TableVisConfig } from '../types'; import { TableVisComponent } from './table_vis_component'; import { TableVisComponentGroup } from './table_vis_component_group'; import { getTableUIState, TableUiState } from '../utils'; +import { VisualizationContainer } from '../../../visualizations/public'; interface TableVisAppProps { services: CoreStart; @@ -36,32 +37,35 @@ export const TableVisApp = ({ }, [handlers]); const tableUiState: TableUiState = getTableUIState(handlers.uiState as PersistedState); + const showNoResult = table ? table.rows.length === 0 : tableGroups?.length === 0; return ( - - {table ? ( - - ) : ( - - )} - + + + {table ? ( + + ) : ( + + )} + + ); diff --git a/src/plugins/vis_type_table/public/table_vis_renderer.tsx b/src/plugins/vis_type_table/public/table_vis_renderer.tsx index 8e467112528..01b995e64fb 100644 --- a/src/plugins/vis_type_table/public/table_vis_renderer.tsx +++ b/src/plugins/vis_type_table/public/table_vis_renderer.tsx @@ -7,7 +7,6 @@ import React from 'react'; import { render, unmountComponentAtNode } from 'react-dom'; import { CoreStart } from 'opensearch-dashboards/public'; -import { VisualizationContainer } from '../../visualizations/public'; import { ExpressionRenderDefinition } from '../../expressions/common/expression_renderers'; import { TableVisRenderValue } from './table_vis_fn'; import { TableVisApp } from './components/table_vis_app'; @@ -23,13 +22,8 @@ export const getTableVisRenderer: ( unmountComponentAtNode(domNode); }); - const showNoResult = visData.table - ? visData.table.rows.length === 0 - : visData.tableGroups?.length === 0; render( - - - , + , domNode ); }, diff --git a/src/plugins/vis_type_table/public/utils/get_table_ui_state.ts b/src/plugins/vis_type_table/public/utils/get_table_ui_state.ts index 58fc6b472a4..67c1a1864f4 100644 --- a/src/plugins/vis_type_table/public/utils/get_table_ui_state.ts +++ b/src/plugins/vis_type_table/public/utils/get_table_ui_state.ts @@ -14,8 +14,8 @@ export interface TableUiState { } export function getTableUIState(uiState: PersistedState): TableUiState { - const sort: ColumnSort = uiState.get('vis.sortColumn') || {}; - const colWidth: ColumnWidth[] = uiState.get('vis.columnsWidth') || []; + const sort: ColumnSort = uiState?.get('vis.sortColumn') || {}; + const colWidth: ColumnWidth[] = uiState?.get('vis.columnsWidth') || []; const setSort = (newSort: ColumnSort) => { uiState.set('vis.sortColumn', newSort);