Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Grid rendering header incorrectly when hiding all children in a group via layout hints #1139

Merged
merged 3 commits into from
Mar 8, 2023
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
6 changes: 4 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,11 @@ npx source-map-explorer 'packages/code-studio/build/static/js/*.js'

## Updating Snapshots

Snapshots are used by end-to-end tests to visually verify the output. Sometimes changes are made requiring snapshots to be updated. Run snapshots locally to update first, by running `npm run e2e:update-snapshots`.
**Note:** You must have [Docker installed](https://docs.docker.com/get-docker/), and `deephaven-core` must already be running on port 10000 on your local machine for all e2e tests.

Once you are satisfied with the snapshots and everything is passing locally, you need to use a docker image to [update snapshots for CI](https://playwright.dev/docs/test-snapshots) (unless you are running the same platform as CI (Ubuntu)). Run `npm run e2e:update-ci-snapshots` to mount the current directory into a docker image and re-run the tests from there. **Note:** You must have [Docker installed](https://docs.docker.com/get-docker/), and `deephaven-core` must already be running on port 10000 on your local machine.
Snapshots are used by end-to-end tests to visually verify the output. Sometimes changes are made requiring snapshots to be updated. Before running snapshots locally, you must be running the development server with `npm start` or have run `npm run build` recently. Run snapshots locally by running `npm run e2e:update-snapshots`.

Once you are satisfied with the snapshots and everything is passing locally, you need to use a docker image to [update snapshots for CI](https://playwright.dev/docs/test-snapshots) (unless you are running the same platform as CI (Ubuntu)). Run `npm run e2e:update-ci-snapshots` which will mount the current directory into a docker image and re-run the tests from there. Test results will be written to your local directories.

## Updating Dependencies

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"e2e:headed": "playwright test --project=chromium --headed --debug",
"e2e:update-snapshots": "playwright test --update-snapshots",
"version-bump": "lerna version --conventional-commits --create-release github",
"e2e:update-ci-snapshots": "docker build --tag web-client-ui-ci-snapshots --file ./tests/update-ci-snapshots/Dockerfile . && docker run --rm --network host --volume $(pwd)/tests:/work/tests/ web-client-ui-ci-snapshots npm run e2e:update-snapshots -- --config=playwright-ci.config.ts"
"e2e:update-ci-snapshots": "docker build --tag web-client-ui-ci-snapshots --file ./tests/update-ci-snapshots/Dockerfile . && docker run --rm --network host --volume $(pwd)/tests:/work/tests/ --volume $(pwd)/playwright-report:/work/playwright-report --volume $(pwd)/test-results:/work/test-results web-client-ui-ci-snapshots npm run e2e:update-snapshots -- --config=playwright-ci.config.ts"
},
"repository": "https://github.com/deephaven/web-client-ui",
"devDependencies": {
Expand Down
7 changes: 2 additions & 5 deletions packages/grid/src/GridRenderer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1592,7 +1592,6 @@ export class GridRenderer {
modelColumns,
allColumnXs,
gridX,
calculatedColumnWidths,
userColumnWidths,
allColumnWidths,
movedColumns,
Expand Down Expand Up @@ -1660,10 +1659,9 @@ export class GridRenderer {
break;
}

// Use this instead of visibleColumnWidths b/c the columns may be off screen
const prevColumnWidth =
userColumnWidths.get(prevModelIndex) ??
calculatedColumnWidths.get(prevModelIndex) ??
allColumnWidths.get(prevColumnIndex) ??
columnWidth;

columnGroupLeft -= prevColumnWidth;
Expand All @@ -1687,10 +1685,9 @@ export class GridRenderer {
break;
}

// Use this instead of visibleColumnWidths b/c the columns may be off screen
const nextColumnWidth =
userColumnWidths.get(nextModelIndex) ??
calculatedColumnWidths.get(nextModelIndex) ??
allColumnWidths.get(nextColumnIndex) ??
columnWidth;

columnGroupRight += nextColumnWidth;
Expand Down
2 changes: 1 addition & 1 deletion playwright.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ const config: PlaywrightTestConfig = {
webServer: {
// Only start the main code-studio server right now
// To test embed-grid and embed-chart, should have an array set for `webServer` and run them all separately as there's a port check
command: 'VITE_PROXY_URL=http://localhost:10000 npm run preview:app',
command: 'npm run preview:app -- -- -- --no-open', // Passing flags through npm is fun
Copy link
Member

Choose a reason for hiding this comment

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

😆

port: 4000,
timeout: 60 * 1000,
reuseExistingServer: !process.env.CI,
Expand Down
31 changes: 31 additions & 0 deletions tests/table.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,37 @@ column_header_group = column_header_group.layout_hints(column_groups=column_grou
await expect(page.locator('.iris-grid-panel .iris-grid')).toHaveScreenshot();
});

test('can open a table with column header groups and hidden columns', async ({
page,
}) => {
await page.goto('');
const consoleInput = page.locator('.console-input');
await consoleInput.click();

const command = `${makeTableCommand('column_header_group')}
column_groups = [{ 'name': 'YandZ', 'children': ['y', 'z'] }, { 'name': 'All', 'children': ['x', 'YandZ'], 'color': 'white' }]
column_header_group = column_header_group.layout_hints(column_groups=column_groups, hide=['y', 'z'])`;

await pasteInMonaco(consoleInput, command);
await page.keyboard.press('Enter');

// Wait for the panel to show
await expect(page.locator('.iris-grid-panel')).toHaveCount(1);

// Wait until it's done loading
await expect(page.locator('.iris-grid-panel .loading-spinner')).toHaveCount(
0
);

// Model is loaded, need to make sure table data is also loaded
await expect(
page.locator('.iris-grid .iris-grid-loading-status')
).toHaveCount(0);

// Now we should be able to check the snapshot
await expect(page.locator('.iris-grid-panel .iris-grid')).toHaveScreenshot();
});

test.describe('tests table operations', () => {
let page: Page;

Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion tests/update-ci-snapshots/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,4 @@ RUN SKIP_POSTINSTALL=1 npm ci
COPY . .

# Now build the app. We only need the code-studio built for e2e tests.
RUN VITE_CORE_API_URL=http://host.docker.internal:10000/jsapi npm run build:app
RUN npm run build:app
Copy link
Member

Choose a reason for hiding this comment

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

Am I dumb? Why does this work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When we were using CRA, there was no proxy server, so if we wanted to point to a JS API not reachable at /jsapi on our preview server, we needed to build it w/ a URL instead.

Prior to #1102 we only proxied requests in dev mode for Vite. With that change, we proxy requests for the Vite preview server as well. So now we can build expecting /jsapi to resolve properly since it will go through the proxy which defaults to localhost:10000