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

feat: increase zoom limits by a factor of 4 #577

Merged
merged 7 commits into from
Apr 19, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
12 changes: 12 additions & 0 deletions client/__tests__/e2e/cellxgeneActions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,18 @@ export async function drag(testId: any, start: any, end: any, lasso = false) {
await page.mouse.up();
}

export async function scroll({testId, deltaY, coords}: {testId: string, deltaY: number, coords: number[]}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is for scrolling

const layout = await waitByID(testId);
if (layout){
const x = coords[0];
const y = coords[1];
await page.mouse.move(x, y);
await page.mouse.down();
await page.mouse.up();
await page.mouse.wheel({ deltaY });
}
}

export async function keyboardUndo() {
await page.keyboard.down("MetaLeft");
await page.keyboard.press("KeyZ");
Expand Down
29 changes: 29 additions & 0 deletions client/__tests__/e2e/e2e.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
calcDragCoordinates,
calcTransformDragCoordinates,
drag,
scroll,
expandCategory,
subset,
createGeneset,
Expand Down Expand Up @@ -58,6 +59,8 @@ import {

import { datasets } from "./data";

import { scaleMax } from "../../src/util/camera";

const BLUEPRINT_SKELETON_CLASS_NAME = Classes.SKELETON;

// geneset CRUD
Expand Down Expand Up @@ -444,6 +447,32 @@ describe("graph overlay", () => {
});
});

test("zoom limit is 12x", async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test colors by and overlays centroid labels to be able to easily get a coordinate overlaying somewhere in the graph.

It then scrolls over that point and gets the test attribute from the graph wrapper to extract the new distance. The scroll was chosen to max out the zoom level, which should equal scaleMax.

Copy link
Contributor

Choose a reason for hiding this comment

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

feels like this comment is super helpful for understanding the test! Can we add it to the code please? Thank you!

await goToPage(pageUrl);

const category = Object.keys(data.categorical)[0];

await clickOn(`colorby-${category}`);
await clickOn("centroid-label-toggle");
await clickOn("mode-pan-zoom");


// @ts-expect-error ts-migrate(7053) FIXME: Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
const categoryValue = Object.keys(data.categorical[category])[0];
const initialCoordinates = await getElementCoordinates(
`${categoryValue}-centroid-label`
);

await tryUntil(async () => {
await scroll({testId: "layout-graph", deltaY: -10000, coords: initialCoordinates});
await page.waitForTimeout(1000);
const newGraph = await page.waitForSelector("[data-test-id^=graph-wrapper-]")
const newGraphTestId = await newGraph?.evaluate((el) => el.getAttribute("data-test-id"))
const newDistance = newGraphTestId?.split("distance=").at(-1);
expect(parseFloat(newDistance)).toBe(scaleMax);
});
});

test("pan zoom mode resets lasso selection", async () => {
await goToPage(pageUrl);

Expand Down
18 changes: 18 additions & 0 deletions client/src/components/graph/graph.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ class Graph extends React.Component<{}, GraphState> {
return !shallowEqual(props.watchProps, prevProps.watchProps);
}

private graphRef = React.createRef<HTMLDivElement>();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is used to prevent scrolling on the graph component at mount time.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is also a great context to have in the code 🙏


// eslint-disable-next-line @typescript-eslint/no-explicit-any --- FIXME: disabled temporarily on migrate to TS.
cachedAsyncProps: any;

Expand Down Expand Up @@ -264,6 +266,11 @@ class Graph extends React.Component<{}, GraphState> {
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
componentDidMount() {
window.addEventListener("resize", this.handleResize);
if (this.graphRef.current) {
this.graphRef.current.addEventListener("wheel", this.disableScroll, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prevent scrolling

passive: false,
});
}
}

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/ban-types --- FIXME: disabled temporarily on migrate to TS.
Expand Down Expand Up @@ -323,6 +330,9 @@ class Graph extends React.Component<{}, GraphState> {
// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
componentWillUnmount() {
window.removeEventListener("resize", this.handleResize);
if (this.graphRef.current) {
this.graphRef.current.removeEventListener("wheel", this.disableScroll);
}
}

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types --- FIXME: disabled temporarily on migrate to TS.
Expand Down Expand Up @@ -496,6 +506,11 @@ class Graph extends React.Component<{}, GraphState> {
});
}

disableScroll = (event: WheelEvent) => {
// disables browser scrolling behavior when hovering over the graph
event.preventDefault();
};

// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types, @typescript-eslint/no-explicit-any -- - FIXME: disabled temporarily on migrate to TS.
setReglCanvas = (canvas: any) => {
// Ignore null canvas on unmount
Expand Down Expand Up @@ -910,6 +925,7 @@ class Graph extends React.Component<{}, GraphState> {
} = this.props;
const { modelTF, projectionTF, camera, viewport, regl } = this.state;
const cameraTF = camera?.view()?.slice();

return (
<div
id="graph-wrapper"
Expand All @@ -918,6 +934,8 @@ class Graph extends React.Component<{}, GraphState> {
top: 0,
left: 0,
}}
data-test-id={`graph-wrapper-distance=${camera?.distance()}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I needed to added the camera's distance into the test attribute. Otherwise, I had no way of figuring out the zoom level since it's a virtual zoom on the coordinates, not a viewport zoom.

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this going to cause unnecessary re-renders?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured if the camera is changing, the graph component would be re-rendered anyway. Let me double check if this is causing extra renders.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't seem to be causing extra renders when zooming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussion with @tihuan, because the graph wrapper div element is re-rendering every time anything changes in the component anyway, including point position due to zoom, then adding dynamic data-test-id isn't causing extra renders. This is also confirmed by adding log statements to the component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for bringing this up, @seve! The style={{...}} is already re-generating a new POJO every time and causing re-rendering, so data-test-id changes is not making React render it more. It's already the most re-rendering React could do on this <div> 😄

ref={this.graphRef}
>
<GraphOverlayLayer
// @ts-expect-error ts-migrate(2322) FIXME: Type '{ children: Element; width: any; height: any... Remove this comment to see the full error message
Expand Down
3 changes: 2 additions & 1 deletion client/src/util/camera.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import clamp from "./clamp";
const EPSILON = 0.000001;

const scaleSpeed = 0.5;
const scaleMax = 3.0;
// exporting this for testing
export const scaleMax = 12.0;
const scaleMin = 0.5;
const panBound = 0.8;

Expand Down