Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/DataGrid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,7 @@ import {
isCtrlKeyHeldDown,
isDefaultCellInput,
renderMeasuringCells,
scrollIntoView,
sign
scrollIntoView
} from './utils';
import type {
CalculatedColumn,
Expand Down Expand Up @@ -573,8 +572,10 @@ export function DataGrid<R, SR = unknown, K extends Key = Key>(props: DataGridPr
previousRowIdx !== rowIdx &&
previousRowIdx < rows.length
) {
const step = sign(rowIdx - previousRowIdx);
for (let i = previousRowIdx + step; i < rowIdx; i += step) {
const [min, max] =
previousRowIdx < rowIdx ? [previousRowIdx, rowIdx] : [rowIdx, previousRowIdx];

for (let i = min + 1; i < max; i++) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a bug Claude found

const row = rows[i];
if (isRowSelectionDisabled?.(row) === true) continue;
if (checked) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ export * from './keyboardUtils';
export * from './renderMeasuringCells';
export * from './styleUtils';

export const { min, max, floor, sign, abs } = Math;
export const { min, max, floor, abs } = Math;

export function assertIsValidKeyGetter<R, K extends React.Key>(
keyGetter: Maybe<(row: NoInfer<R>) => K>
Expand Down
9 changes: 3 additions & 6 deletions test/browser/column/resizable.test.tsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useState } from 'react';
import { commands, page, userEvent } from 'vitest/browser';
import { commands, page, server, userEvent } from 'vitest/browser';

import { DataGrid, type Column, type ColumnWidth, type ColumnWidths } from '../../../src';
import { setup } from '../utils';
Expand Down Expand Up @@ -313,11 +313,8 @@ test('should use columnWidths and onColumnWidthsChange props when provided', asy
});

async function testGridTemplateColumns(chrome: string, firefox: string, firefoxCI = firefox) {
const gridTemplateColumns = navigator.userAgent.includes('Chrome')
? chrome
: __IS_CI__
? firefoxCI
: firefox;
const gridTemplateColumns =
server.browser === 'chromium' ? chrome : import.meta.env.CI ? firefoxCI : firefox;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking server.browser instead of navigator.userAgent seems safer.
Also moved CI check to import.meta.env.CI instead of defining a global variable, seems more idiomatic.


await expect.element(grid).toHaveStyle({ gridTemplateColumns });
}
18 changes: 18 additions & 0 deletions test/browser/rowSelection.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -239,16 +239,34 @@ test('extra keys are preserved when updating the selectedRows Set', async () =>

test('select/deselect rows using shift click', async () => {
await setup();

// forward selection
await toggleSelection(0);
await toggleSelection(2, true);
await testSelection(0, true);
await testSelection(1, true);
await testSelection(2, true);

// forward deselection
await toggleSelection(0);
await toggleSelection(2, true);
await testSelection(0, false);
await testSelection(1, false);
await testSelection(2, false);

// backward selection
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

await toggleSelection(2);
await toggleSelection(0, true);
await testSelection(0, true);
await testSelection(1, true);
await testSelection(2, true);

// backward deselection
await toggleSelection(2);
await toggleSelection(0, true);
await testSelection(0, false);
await testSelection(1, false);
await testSelection(2, false);
});

test('select rows using shift space', async () => {
Expand Down
15 changes: 13 additions & 2 deletions test/browser/virtualization.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -220,18 +220,29 @@ test('zero rows', async () => {
await expect.element(rows).toHaveLength(0);
});

test('virtualization is enable with not enough columns or rows to virtualize', async () => {
test('virtualization is enabled with not enough columns or rows to virtualize', async () => {
await setupGrid(true, 5, 5);

await assertHeaderCells(5, 0, 4);
await assertRows(5, 0, 4);
await expect.element(cells).toHaveLength(5 * 5);
});

test('enableVirtualization is disabled', async () => {
test('virtualization is disabled with no frozen columns', async () => {
await setupGrid(false, 40, 100);

await assertHeaderCells(40, 0, 39);
await assertRows(100, 0, 99);
await expect.element(cells).toHaveLength(40 * 100);
});

// failing test
// cannot use `test.fails` as console logs lead to timeout in parallel tests
// https://github.com/vitest-dev/vitest/issues/9941
test.skip('virtualization is disabled with some frozen columns', async () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no virtualization + frozen columns = we render frozen columns multiple times, leading to duplicate key warnings.
I'll fix it in a separate PR

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

await setupGrid(false, 40, 100, 3);

await assertHeaderCells(40, 0, 39);
await assertRows(100, 0, 99);
await expect.element(cells).toHaveLength(40 * 100);
});
46 changes: 23 additions & 23 deletions test/failOnConsole.ts
Original file line number Diff line number Diff line change
@@ -1,37 +1,37 @@
let consoleErrorOrConsoleWarnWereCalled = false;
beforeEach(({ onTestFinished }) => {
vi.spyOn(console, 'warn').mockName('console.warn');

beforeAll(() => {
console.error = (...params) => {
// use split mocks to not increase the calls count when ignoring undesired logs
const errorMock = vi.fn(console.error).mockName('console.error');
vi.spyOn(console, 'error').mockImplementation(function error(...params) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// https://github.com/vitest-dev/vitest/blob/0685b6f027576589464fc6109ddc071ef0079f16/packages/browser/src/client/public/error-catcher.js#L34-L38
// https://github.com/vitest-dev/vitest/blob/0685b6f027576589464fc6109ddc071ef0079f16/test/browser/fixtures/unhandled-non-error/basic.test.ts
if (
params[0] instanceof Error &&
params[0].message === 'ResizeObserver loop completed with undelivered notifications.'
) {
return;
}

consoleErrorOrConsoleWarnWereCalled = true;
console.log(...params);
};

console.warn = (...params) => {
consoleErrorOrConsoleWarnWereCalled = true;
console.log(...params);
};
});
return errorMock(...params);
});

afterEach(() => {
// Wait for the test and all `afterEach` hooks to complete to ensure all logs are caught
onTestFinished(({ task, signal }) => {
onTestFinished(({ expect, task, signal }) => {
// avoid failing test runs twice
if (task.result!.state !== 'fail' && !signal.aborted) {
expect
.soft(
consoleErrorOrConsoleWarnWereCalled,
'errors/warnings were logged to the console during the test'
)
.toBe(false);
}
if (task.result?.state === 'fail' || signal.aborted) return;

consoleErrorOrConsoleWarnWereCalled = false;
expect
.soft(
console.warn,
'console.warn() was called during the test; please resolve unexpected warnings'
)
.toHaveBeenCalledTimes(0);
expect
.soft(
errorMock,
'console.error() was called during the test; please resolve unexpected errors'
)
.toHaveBeenCalledTimes(0);
});
});
6 changes: 5 additions & 1 deletion test/globals.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
declare global {
const __IS_CI__: boolean;
interface ImportMeta {
readonly env: {
readonly CI: boolean;
};
}
}

declare module 'vitest/browser' {
Expand Down
125 changes: 59 additions & 66 deletions vite.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ import react from '@vitejs/plugin-react';
import { playwright, type PlaywrightProviderOptions } from '@vitest/browser-playwright';
import { ecij } from 'ecij/plugin';
import { defineConfig, type ViteUserConfig } from 'vitest/config';
import type { BrowserCommand, BrowserInstanceOption } from 'vitest/node';
import type { BrowserCommand } from 'vitest/node';

const isCI = process.env.CI === 'true';
const isTest = process.env.NODE_ENV === 'test';
const isTest = process.env.VITEST === 'true';
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://vitest.dev/guide/#configuring-vitest
Seems to be the recommended variable to check against

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL


// TODO: remove when `userEvent.pointer` is supported
const resizeColumn: BrowserCommand<[name: string, resizeBy: number | readonly number[]]> = async (
Expand Down Expand Up @@ -48,59 +48,42 @@ const playwrightOptions: PlaywrightProviderOptions = {
}
};

// vitest modifies the instance objects, so we cannot rely on static objects
// https://github.com/vitest-dev/vitest/issues/9877
function getInstances(): BrowserInstanceOption[] {
return [
{
browser: 'chromium',
provider: playwright({
...playwrightOptions,
launchOptions: {
channel: 'chromium'
}
})
},
{
browser: 'firefox',
provider: playwright(playwrightOptions),
// TODO: remove when FF tests are stable
fileParallelism: false
}
];
}

export default defineConfig(
({ isPreview }): ViteUserConfig => ({
base: '/react-data-grid/',
cacheDir: '.cache/vite',
clearScreen: false,
define: isTest ? { __IS_CI__: JSON.stringify(isCI) } : {},
build: {
modulePreload: { polyfill: false },
sourcemap: true,
reportCompressedSize: false,
// https://github.com/parcel-bundler/lightningcss/issues/873
cssTarget: 'esnext'
},
plugins: [
ecij(),
(!isTest || isPreview) &&
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check wasn't working correctly...

tanstackRouter({
target: 'react',
generatedRouteTree: 'website/routeTree.gen.ts',
routesDirectory: 'website/routes',
autoCodeSplitting: true
}),
react()
],
plugins: isPreview
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now properly disable plugins when running preview

? []
: [
ecij(),
!isTest &&
tanstackRouter({
target: 'react',
generatedRouteTree: 'website/routeTree.gen.ts',
routesDirectory: 'website/routes',
autoCodeSplitting: true
}),
react()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure react() does anything when running tests... tests work fine without it, I left it just in case.

],
server: {
open: true
},
test: {
dir: 'test',
globals: true,
printConsoleTrace: true,
env: {
// @ts-expect-error
CI: isCI
},
coverage: {
provider: 'istanbul',
enabled: isCI,
Expand All @@ -120,20 +103,51 @@ export default defineConfig(
}
},
slowTestThreshold: 1000,
browser: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deduped browser settings

headless: true,
ui: false,
viewport,
commands: { resizeColumn, dragFill },
expect: {
toMatchScreenshot: {
resolveScreenshotPath({
root,
testFileDirectory,
testFileName,
arg,
browserName,
platform,
ext
}) {
return `${root}/${testFileDirectory}/screenshots/${testFileName}/${arg}-${browserName}-${platform}${ext}`;
}
}
},
instances: [
{
browser: 'chromium',
provider: playwright({
...playwrightOptions,
launchOptions: {
channel: 'chromium'
}
})
},
{
browser: 'firefox',
provider: playwright(playwrightOptions),
// TODO: remove when FF tests are stable
fileParallelism: false
}
]
},
projects: [
{
extends: true,
test: {
name: 'browser',
include: ['browser/**/*.test.*'],
browser: {
enabled: true,
instances: getInstances(),
commands: { resizeColumn, dragFill },
viewport,
headless: true,
ui: false
},
browser: { enabled: true },
setupFiles: ['test/browser/styles.css', 'test/setupBrowser.ts', 'test/failOnConsole.ts']
}
},
Expand All @@ -142,28 +156,7 @@ export default defineConfig(
test: {
name: 'visual',
include: ['visual/*.test.*'],
browser: {
enabled: true,
instances: getInstances(),
viewport,
headless: true,
ui: false,
expect: {
toMatchScreenshot: {
resolveScreenshotPath({
root,
testFileDirectory,
testFileName,
arg,
browserName,
platform,
ext
}) {
return `${root}/${testFileDirectory}/screenshots/${testFileName}/${arg}-${browserName}-${platform}${ext}`;
}
}
}
},
browser: { enabled: true },
setupFiles: ['test/setupBrowser.ts', 'test/failOnConsole.ts']
}
},
Expand Down