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

atarashansky · 2023-03-23T19:11:45Z

Changes

increase zoom limit to 12x from 3x
add smoke test for zoom
prevent scrolling on graph component

Testing

tested locally and smoke tested

Notes for reviewer

Previously, scrolling behavior conflicts with zoom tool:
https://user-images.githubusercontent.com/16548075/231508562-a850bf9c-8861-442f-bdbc-122236288540.mov

Now, the rubberbanding scrolling behavior is no longer present on the graph component:
https://user-images.githubusercontent.com/16548075/231508671-2467c602-80b2-4d17-afd9-42eee2b294a2.mov

codecov · 2023-04-10T17:09:00Z

Codecov Report

Merging #577 (88f2893) into main (260354e) will not change coverage.
The diff coverage is n/a.

❗ Current head 88f2893 differs from pull request most recent head 8cb0562. Consider uploading reports for the commit 8cb0562 to get more accurate results

@@           Coverage Diff           @@
##             main     #577   +/-   ##
=======================================
  Coverage   78.40%   78.40%           
=======================================
  Files          84       84           
  Lines        6594     6594           
=======================================
  Hits         5170     5170           
  Misses       1424     1424

Flag	Coverage Δ
frontend	`78.40% <ø> (ø)`
javascript	`78.40% <ø> (ø)`
smokeTest	`∅ <ø> (∅)`
unitTest	`78.40% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

atarashansky · 2023-04-11T22:54:37Z

client/src/components/graph/graph.tsx

@@ -918,6 +918,7 @@ class Graph extends React.Component<{}, GraphState> {
          top: 0,
          left: 0,
        }}
+        data-test-id={`graph-wrapper-distance=${camera?.distance()}`}


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.

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

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.

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

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.

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> 😄

atarashansky · 2023-04-11T22:56:06Z

client/__tests__/e2e/cellxgeneActions.ts

@@ -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[]}) {


This function is for scrolling

atarashansky · 2023-04-11T22:57:27Z

client/__tests__/e2e/e2e.test.ts

@@ -444,6 +447,32 @@ describe("graph overlay", () => {
  });
 });

+test("zoom limit is 12x", async () => {


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.

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

seve · 2023-04-11T23:29:50Z

client/src/components/graph/graph.tsx

@@ -918,6 +918,7 @@ class Graph extends React.Component<{}, GraphState> {
          top: 0,
          left: 0,
        }}
+        data-test-id={`graph-wrapper-distance=${camera?.distance()}`}


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

atarashansky · 2023-04-12T15:42:20Z

client/src/components/graph/graph.tsx

@@ -126,6 +126,8 @@ class Graph extends React.Component<{}, GraphState> {
    return !shallowEqual(props.watchProps, prevProps.watchProps);
  }

+  private graphRef = React.createRef<HTMLDivElement>();


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

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

atarashansky · 2023-04-12T15:42:40Z

client/src/components/graph/graph.tsx

@@ -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, {


prevent scrolling

tihuan

LGTM! Just proposing to add some PR comments to code directly to beef up code context 🙏 Thanks so much!

tihuan · 2023-04-13T21:02:02Z

client/__tests__/e2e/e2e.test.ts

@@ -444,6 +447,32 @@ describe("graph overlay", () => {
  });
 });

+test("zoom limit is 12x", async () => {


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

tihuan · 2023-04-13T21:02:32Z

client/src/components/graph/graph.tsx

@@ -126,6 +126,8 @@ class Graph extends React.Component<{}, GraphState> {
    return !shallowEqual(props.watchProps, prevProps.watchProps);
  }

+  private graphRef = React.createRef<HTMLDivElement>();


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

* feat: increase zoom limits by a factor of 4 * add zoom test * remove extraneous * prevent scrolling when hovering over graph * remove import --------- Co-authored-by: atarashansky <[email protected]> Co-authored-by: atarashansky <[email protected]>

feat: increase zoom limits by a factor of 4

11ab7e0

atarashansky marked this pull request as draft March 24, 2023 17:32

atarashansky changed the title ~~feat: increase zoom limits by a factor of 4~~ (DO NOT MERGE) feat: increase zoom limits by a factor of 4 Mar 24, 2023

atarashansky mentioned this pull request Apr 6, 2023

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

Closed

Merge branch 'main' into atar-increase-zoom-rdev

0e21819

add zoom test

2e3e03e

atarashansky changed the title ~~(DO NOT MERGE) feat: increase zoom limits by a factor of 4~~ feat: increase zoom limits by a factor of 4 Apr 11, 2023

atarashansky requested a review from tihuan April 11, 2023 22:53

atarashansky marked this pull request as ready for review April 11, 2023 22:53

atarashansky commented Apr 11, 2023

View reviewed changes

remove extraneous

8a9caff

atarashansky commented Apr 11, 2023

View reviewed changes

atarashansky requested a review from seve April 11, 2023 22:58

seve suggested changes Apr 11, 2023

View reviewed changes

atarashansky added 2 commits April 12, 2023 11:31

prevent scrolling when hovering over graph

53deb97

remove import

4788237

atarashansky requested a review from seve April 12, 2023 15:38

atarashansky commented Apr 12, 2023

View reviewed changes

Merge branch 'main' into atar-increase-zoom-rdev

8cb0562

ashin-czi approved these changes Apr 13, 2023

View reviewed changes

seve approved these changes Apr 13, 2023

View reviewed changes

tihuan approved these changes Apr 13, 2023

View reviewed changes

atarashansky merged commit db5051d into main Apr 19, 2023

atarashansky deleted the atar-increase-zoom-rdev branch April 19, 2023 20:13

czi-github-helper bot mentioned this pull request Jun 20, 2024

chore(main): release 1.0.0 #995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

atarashansky commented Mar 23, 2023 •

edited

Loading

codecov bot commented Apr 10, 2023 •

edited

Loading

atarashansky Apr 11, 2023

seve Apr 11, 2023

atarashansky Apr 12, 2023

atarashansky Apr 12, 2023

atarashansky Apr 12, 2023

tihuan Apr 12, 2023

atarashansky Apr 11, 2023

atarashansky Apr 11, 2023

tihuan Apr 13, 2023

seve Apr 11, 2023

atarashansky Apr 12, 2023

tihuan Apr 13, 2023

atarashansky Apr 12, 2023

tihuan left a comment

tihuan Apr 13, 2023

tihuan Apr 13, 2023

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

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

Conversation

atarashansky commented Mar 23, 2023 • edited Loading

Changes

Testing

Notes for reviewer

codecov bot commented Apr 10, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tihuan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atarashansky commented Mar 23, 2023 •

edited

Loading

codecov bot commented Apr 10, 2023 •

edited

Loading