rusackas commented on code in PR #42248:
URL: https://github.com/apache/superset/pull/42248#discussion_r3997103903
##########
superset-frontend/plugins/plugin-chart-table/src/DataTable/DataTable.tsx:
##########
@@ -461,11 +461,20 @@ export default typedMemo(function DataTable<D extends
object>({
resultPageCount = 0;
}
resultCurrentPageSize = serverPageSize;
- const foundPageSizeIndex = pageSizeOptions.findIndex(
- ([option]) => option >= resultCurrentPageSize,
+ const exactMatch = pageSizeOptions.some(
+ ([option]) => option === resultCurrentPageSize,
);
- if (foundPageSizeIndex === -1) {
- resultCurrentPageSize = 0;
+ if (!exactMatch) {
+ const nearestOption = pageSizeOptions.find(
+ ([option]) => option >= resultCurrentPageSize,
+ );
+ if (nearestOption) {
+ resultCurrentPageSize = nearestOption[0];
+ } else if (pageSizeOptions.length > 0) {
+ resultCurrentPageSize = pageSizeOptions[pageSizeOptions.length - 1][0];
+ } else {
+ resultCurrentPageSize = 0;
Review Comment:
This is fixed by b966441 ("fix(plugin-chart-table): keep fallback page size
in sync with server callback"). Both `resultPageCount` (line 478) and
`resultOnPageChange` (line 483-484) now consistently use
`resultCurrentPageSize`, the fallback-adjusted value, so the selector and
server pagination/page-count no longer desync. Confirmed at current head.
Resolving.
##########
superset-frontend/plugins/plugin-chart-table/test/TableChart.test.tsx:
##########
@@ -2482,6 +2482,47 @@ describe('plugin-chart-table', () => {
);
expect(screen.queryByText('Search by')).toBeInTheDocument();
});
+
+ test('dropdown preserves serverPageLength if larger than rowCount and avoids
invalid 0 (#42243)', async () => {
+ const props = transformProps({
+ ...testData.basic,
+ formData: {
+ ...testData.basic.formData,
+ server_pagination: true,
+ },
+ });
+ props.serverPagination = true;
+ props.serverPageLength = 20;
+ props.rowCount = 12;
+ props.data = Array.from({ length: 12 }, (_, i) => ({
+ name: `Row ${i}`,
+ })) as any;
+
+ const { container } = render(
+ <ProviderWrapper>
+ <TableChart {...props} sticky={false} />
+ </ProviderWrapper>,
+ );
+
+ // Initial page size selector text
+ const pageSizeSelector = container.querySelector('.dt-select-page-size');
+ expect(pageSizeSelector).toHaveTextContent('20');
+
+ // Open dropdown to check options
+ const selectTrigger = container.querySelector('.ant-select-selector');
+ if (selectTrigger) {
+ fireEvent.mouseDown(selectTrigger);
+
+ await waitFor(() => {
+ const options = screen.getAllByRole('option');
+ const optionTexts = options.map(opt => opt.textContent);
+ expect(optionTexts).toContain('10');
+ expect(optionTexts).toContain('20');
+ expect(optionTexts).not.toContain('0');
+ expect(optionTexts).not.toContain('All');
+ });
+ }
Review Comment:
Fixed in c469e4b ("fix(table): use accessible combobox query for page size
selector (#42243)"). The test now does `screen.getByRole('combobox', { name:
'Show entries per page' })`, which throws if the selector isn't found, plus an
explicit `expect(selectTrigger).toBeInTheDocument()`. No more `if
(selectTrigger)` guard. Confirmed at current head. Resolving.
--
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]
