sadpandajoe commented on code in PR #36081: URL: https://github.com/apache/superset/pull/36081#discussion_r3463277118
########## superset-frontend/plugins/plugin-chart-pivot-table/test/testData.ts: ########## @@ -0,0 +1,156 @@ +/** + * 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 { + ChartDataResponseResult, + ChartProps, + DatasourceType, + VizType, + QueryFormData, +} from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; +import { GenericDataType } from '@apache-superset/core/common'; + +const basicFormData: QueryFormData = { + datasource: '1__table', + viz_type: VizType.PivotTable, + groupbyRows: ['country'], Review Comment: This fixture uses a single row dimension with one row per group, so every row label has rowSpan 1 and the merged-cell case never gets tested. Issue #36031 is the nested case (last group expanded, outer label spanning multiple rows), so could we add a two-dimension fixture that would fail if the merged-cell border regresses? ########## superset-frontend/plugins/plugin-chart-pivot-table/test/TableRenderer.test.tsx: ########## @@ -0,0 +1,91 @@ +/** + * 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 '@testing-library/jest-dom'; +import { render } from '@testing-library/react'; +import PivotTableChart from '../src/PivotTableChart'; +import transformProps from '../src/plugin/transformProps'; +import testData from './testData'; +import { ProviderWrapper } from './testHelpers'; + +test('applies pvtRowLabelLast class to last data row when colTotals is disabled', () => { + const transformedProps = { + ...transformProps(testData.withoutColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const tableBody = container.querySelector('tbody'); + const dataRows = Array.from(tableBody?.querySelectorAll('tr') ?? []).filter( + row => !row.classList.contains('pvtRowTotals'), + ); + + // Get the last data row + const lastDataRow = dataRows[dataRows.length - 1]; + expect(lastDataRow).toBeInTheDocument(); + + // Check if any cell in the last data row has pvtRowLabelLast class + const lastRowCells = lastDataRow.querySelectorAll('th.pvtRowLabel'); + const hasLastClass = Array.from(lastRowCells).some(cell => + cell.classList.contains('pvtRowLabelLast'), + ); + + expect(hasLastClass).toBe(true); +}); + +test('does not apply pvtRowLabelLast class to last data row when colTotals is enabled', () => { + const transformedProps = { + ...transformProps(testData.withColTotals), + margin: 32, + legacy_order_by: null, + order_desc: false, + }; + const { container } = render( + ProviderWrapper({ + children: <PivotTableChart {...transformedProps} />, + }), + ); + + const tableBody = container.querySelector('tbody'); + const dataRows = Array.from(tableBody?.querySelectorAll('tr') ?? []).filter( + row => !row.classList.contains('pvtRowTotals'), + ); + + // Get the last data row (before totals row) + const lastDataRow = dataRows[dataRows.length - 1]; + expect(lastDataRow).toBeInTheDocument(); + + // Check if any cell in the last data row has pvtRowLabelLast class + const lastRowCells = lastDataRow.querySelectorAll('th.pvtRowLabel'); + const hasLastClass = Array.from(lastRowCells).some(cell => + cell.classList.contains('pvtRowLabelLast'), + ); + + // Should NOT have the class because totals row will have the border + expect(hasLastClass).toBe(false); Review Comment: This only checks the class is absent, which stays true even if the fix is reverted entirely, so it doesn't pin the colTotals behavior. Could we also assert the totals row carries the bottom border here? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
