feat: 🎸 Add Industry Comparison Section #427

Malayvasa · 2024-07-30T10:30:09Z

Related to #381

iamacook · 2024-07-31T08:39:08Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+    const updateDimensions = () => {
+      const newDimensions = updateCanvasDimensions(canvasRef, containerRef)
+      setDimensions(newDimensions)
+    }
+
+    const handleMouseMove = (e: MouseEvent) => {
+      if (canvasRef.current && containerRef.current) {
+        const rect = containerRef.current.getBoundingClientRect()
+        setMousePosition({ x: e.clientX - rect.left, y: e.clientY - rect.top })
+      }
+    }
+
+    const updateMobileMousePosition = () => {
+      if (scrollYProgress && isMobile && containerRef.current) {
+        const progress = scrollYProgress.get()
+        const { height } = containerRef.current.getBoundingClientRect()
+        setMousePosition({ x: dimensions.width - dimensions.width / 8, y: progress * height })
+      }
+    }
+
+    const lerpMousePosition = () => {
+      setLerpedMousePosition((prev) => ({
+        x: lerp(prev.x, mousePosition.x),
+        y: lerp(prev.y, mousePosition.y),
+      }))
+      requestAnimationFrame(lerpMousePosition)
+    }


Due to the dependencies, I'd suggest we move memoise these in useCallbacks.

Addressed in 01d5a3b

iamacook · 2024-07-31T08:40:13Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+    }
+
+    const handleMouseMove = (e: MouseEvent) => {
+      if (canvasRef.current && containerRef.current) {


Is canvasRef.current required for the mouse position? The value isn't used inside the handler.

Addressed in fffb8ab

iamacook · 2024-07-31T08:41:02Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+      if (canvasRef.current && containerRef.current) {
+        const rect = containerRef.current.getBoundingClientRect()
+        setMousePosition({ x: e.clientX - rect.left, y: e.clientY - rect.top })
+      }
+    }
+
+    const updateMobileMousePosition = () => {
+      if (scrollYProgress && isMobile && containerRef.current) {


We could use early returns here to reduce indentation, e.g.

Suggested change

if (canvasRef.current && containerRef.current) {

const rect = containerRef.current.getBoundingClientRect()

setMousePosition({ x: e.clientX - rect.left, y: e.clientY - rect.top })

}

}

const updateMobileMousePosition = () => {

if (scrollYProgress && isMobile && containerRef.current) {

if (canvasRef.current && containerRef.current) return

const rect = containerRef.current.getBoundingClientRect()

setMousePosition({ x: e.clientX - rect.left, y: e.clientY - rect.top })

}

const updateMobileMousePosition = () => {

if (scrollYProgress && isMobile && containerRef.current) return

Addressed in 01d5a3b

iamacook · 2024-07-31T08:42:56Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+        x: lerp(prev.x, mousePosition.x),
+        y: lerp(prev.y, mousePosition.y),
+      }))
+      requestAnimationFrame(lerpMousePosition)


We need to clean this up with cancelAnimationFrame on unmount.

Addressed in 01d5a3b

iamacook · 2024-07-31T08:44:34Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+  useEffect(() => {
+    const updateDimensions = () => {
+      const newDimensions = updateCanvasDimensions(canvasRef, containerRef)
+      setDimensions(newDimensions)
+    }
+
+    const handleMouseMove = (e: MouseEvent) => {
+      if (canvasRef.current && containerRef.current) {
+        const rect = containerRef.current.getBoundingClientRect()
+        setMousePosition({ x: e.clientX - rect.left, y: e.clientY - rect.top })
+      }
+    }
+
+    const updateMobileMousePosition = () => {
+      if (scrollYProgress && isMobile && containerRef.current) {
+        const progress = scrollYProgress.get()
+        const { height } = containerRef.current.getBoundingClientRect()
+        setMousePosition({ x: dimensions.width - dimensions.width / 8, y: progress * height })
+      }
+    }
+
+    const lerpMousePosition = () => {
+      setLerpedMousePosition((prev) => ({
+        x: lerp(prev.x, mousePosition.x),
+        y: lerp(prev.y, mousePosition.y),
+      }))
+      requestAnimationFrame(lerpMousePosition)
+    }
+
+    updateDimensions()
+    window.addEventListener('resize', updateDimensions)
+    const container = containerRef.current
+    if (isMobile && scrollYProgress) {
+      scrollYProgress.onChange(updateMobileMousePosition)
+    } else {
+      container?.addEventListener('mousemove', handleMouseMove)
+    }
+    lerpMousePosition()
+
+    return () => {
+      window.removeEventListener('resize', updateDimensions)
+      if (isMobile && scrollYProgress) {
+        scrollYProgress.clearListeners()
+      } else {
+        container?.removeEventListener('mousemove', handleMouseMove)
+      }
+    }
+  }, [mousePosition.x, mousePosition.y, containerRef, isMobile, scrollYProgress, dimensions.width])


There's a lot going on here in one useEffect. If it's possible, breaking it down would be ideal. Memoising the methods in useCallbacks would help.

Addressed in 01d5a3b

iamacook · 2024-07-31T08:45:52Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+    const lerpMousePosition = () => {
+      setLerpedMousePosition((prev) => ({
+        x: lerp(prev.x, mousePosition.x),
+        y: lerp(prev.y, mousePosition.y),
+      }))
+      requestAnimationFrame(lerpMousePosition)
+    }


Afaiu, this is related to drawing the dots? Could we locate them together?

Addressed in drawDots util in 01d5a3b

iamacook · 2024-07-31T08:48:42Z

src/components/DataRoom/IndustryComparison/utils/updateCanvasDimensions.ts

+  canvasRef: React.RefObject<HTMLCanvasElement>,
+  containerRef: React.RefObject<HTMLDivElement>,


Instead of passing the ref, I think we can use the element directly, e.g.

Suggested change

canvasRef: React.RefObject<HTMLCanvasElement>,

containerRef: React.RefObject<HTMLDivElement>,

canvas: HTMLCanvasElement,

container: HTMLDivElement,

Addressed in updateCanvas util in 01d5a3b

iamacook · 2024-07-31T08:51:27Z

src/components/DataRoom/IndustryComparison/utils/updateCanvasDimensions.ts

+export const updateCanvasDimensions = (
+  canvasRef: React.RefObject<HTMLCanvasElement>,
+  containerRef: React.RefObject<HTMLDivElement>,
+) => {
+  if (canvasRef.current && containerRef.current) {
+    const { clientWidth, clientHeight } = containerRef.current
+    const pixelRatio = window.devicePixelRatio || 1
+    canvasRef.current.width = clientWidth * pixelRatio
+    canvasRef.current.height = clientHeight * pixelRatio
+    canvasRef.current.style.width = `${clientWidth}px`
+    canvasRef.current.style.height = `${clientHeight}px`
+
+    const ctx = canvasRef.current.getContext('2d')
+    if (ctx) {
+      ctx.scale(pixelRatio, pixelRatio)
+    }
+    return { width: clientWidth, height: clientHeight }
+  }
+  return { width: 0, height: 0 }
+}


I'd suggest we split this - it's updating the canvas, but also getting the dimensions of the container (used in the component). I'd suggest we have two functions: something like getContainerDimensions (and maybe we don't need to store that in state) and then a function to update the canvas.

Addressed by using useContainerSize hook in 01d5a3b

Malayvasa · 2024-08-01T14:04:15Z

@iamacook I have addressed all comments I could that did not break the animation.

DiogoSoaress · 2024-08-05T14:23:16Z

src/components/DataRoom/IndustryComparison/index.tsx

+const RightPanel = ({
+  scrollYProgress,
+  children,
+  containerRef,
+  isMobile,
+}: {
+  scrollYProgress: MotionValue<number>
+  children: ReactNode
+  isMobile: boolean
+  containerRef: React.RefObject<HTMLDivElement>
+}) => {
+  const opacityParams = [0.25, 0.35, 0.65, 0.75]
+  const translateParams = [0.25, 0.35, 0.65, 0.75]
+  const opacity = useTransform(scrollYProgress, opacityParams, [0, 1, 1, 0])
+  const bgTranslate = useTransform(scrollYProgress, translateParams, ['100%', '0%', '0%', '100%'])
+
+  return (
+    <div ref={containerRef} className={css.rightPanelContainer}>
+      <motion.div
+        className={css.rightPanelContent}
+        style={{
+          opacity: isMobile ? 1 : opacity,
+        }}
+      >
+        {children}
+      </motion.div>
+      <motion.div
+        className={css.rightPanelBG}
+        style={{
+          translateX: isMobile ? '0%' : bgTranslate,
+        }}
+      ></motion.div>
+    </div>
+  )
+}


Can we consolidate this component with the one used in the CryptoPunks section? They seem almost identical.

I would suggest to move this component under the /common folder and name it something more descriptive. Example SlidingRightPanel. WDYT?

Not worth doing imo, both the panels have different behaviour for desktop and different behaviour for mobile view and different styling for the content.

DiogoSoaress · 2024-08-05T14:24:29Z

src/components/DataRoom/IndustryComparison/styles.module.css

+  font-size: 80px;
+  line-height: 88px;


These values are already defined in the h1 variant. I don't think we need this class

Addressed in 01d5a3b

You can delete the class. Why do you need the z-index here?

src/components/DataRoom/IndustryComparison/utils/lerp.ts

src/components/DataRoom/IndustryComparison/utils/updateCanvasDimensions.ts

src/components/DataRoom/IndustryComparison/DotGrid.tsx

src/components/DataRoom/IndustryComparison/utils/createDots.ts

src/components/DataRoom/IndustryComparison/utils/drawDots.ts

src/components/DataRoom/IndustryComparison/DotGrid.tsx

DiogoSoaress

Detected some causes of performance issues

src/components/DataRoom/IndustryComparison/styles.module.css

DiogoSoaress · 2024-08-15T13:27:48Z

src/components/DataRoom/IndustryComparison/Content.tsx

+        opacity: isMobile ? 1 : opacity,
+      }}
+    >
+      <Typography className={css.title} variant="h1">


You don't need this title class, do you?

I need it to apply z-index and pointer-events properties. Will remove any font size attributes.

Addressed in 9f82803

src/components/common/SlidingPanel/index.tsx

src/components/DataRoom/IndustryComparison/utils/drawDots.ts

DiogoSoaress · 2024-08-15T14:12:38Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+
+    const animate = () => {
+      if (dimensions.width > 0 && dimensions.height > 0) {
+        drawDots(ctx, dots, dimensions, mousePosition, isMobile)


This function is expensive to be called non-stop. Can you only redraw the dots when necessary? (i.e, when the mouse moves or when the resolution change)

Can you also explore a way to not have to draw the whole grid when the mouse moves but only the dots that are changing size?

Addressed in 9f82803

How did you address it?

src/components/DataRoom/IndustryComparison/styles.module.css

src/components/DataRoom/IndustryComparison/Content.tsx

DiogoSoaress · 2024-08-15T14:29:14Z

src/components/DataRoom/IndustryComparison/DotGrid.tsx

+    if (!ctx) return
+
+    const dots = createDots(dimensions, isMobile)
+    updateCanvas(canvas, ctx, dimensions.width, dimensions.height)


This is also being called unconditionally without any change in the dimensions.

Addressed in 9f82803

How did you address it ? drawDots still being called unconditionally at every possible frame and updating the canvas context for every dot

src/components/DataRoom/IndustryComparison/utils/useMousePosition.ts

DiogoSoaress

There are still some performance considerations:

The animation loop runs continuously, which can be CPU-intensive. Consider adding throttling or debouncing if the animation doesn't need to run at the highest possible frame rate.
Canvas Updates: The canvas is updated on every frame. Ensure that drawDots function is optimized to minimize the amount of work done per frame.

Malayvasa · 2024-08-20T10:08:17Z

I have moved the mouse hover effect in the DotGrid component to PR #442 as it is not performant yet.

iamacook and others added 2 commits July 25, 2024 10:44

Open branch

918350c

feat: 🎸 Add Industry Comparison Section

c4aebba

iamacook reviewed Jul 31, 2024

View reviewed changes

fix: 🐛 address PR comments

c119de2

Malayvasa requested a review from iamacook August 1, 2024 14:03

Malayvasa changed the base branch from malay-branch-1 to main August 2, 2024 15:14

DiogoSoaress requested changes Aug 5, 2024

View reviewed changes

Malayvasa added 2 commits August 13, 2024 13:06

Merge branch 'main' into industry-comparison

1a6f721

refactor: 💡 address PR comments

01d5a3b

Malayvasa requested a review from DiogoSoaress August 13, 2024 08:51

Malayvasa added 4 commits August 13, 2024 14:29

docs: ✏️ add descripition for useMousePosition hook

ccf8dfe

refactor: 💡 remove container ref from useMousePosition

fffb8ab

refactor: 💡 add SlidingPanel component and update Cryptopunks

74afb38

refactor: 💡 dynamic import for framer motion

a8a5d94

DiogoSoaress requested changes Aug 15, 2024

View reviewed changes

refactor: 💡 address PR comments

9f82803

Malayvasa requested a review from DiogoSoaress August 15, 2024 16:28

DiogoSoaress requested changes Aug 19, 2024

View reviewed changes

Malayvasa added 5 commits August 19, 2024 21:04

refactor: 💡 conditionally call drawDots to improve performance

27b64ca

style: 💄 remove unused title style

3daff57

Merge branch 'main' into industry-comparison

42bee79

refactor: 💡 remove unused prop dimensions from drawDots

07dd56a

perf: ⚡️ remove mouse hover effect from dot grid animation

4e7f180

Malayvasa requested a review from DiogoSoaress August 20, 2024 10:33

DiogoSoaress approved these changes Aug 20, 2024

View reviewed changes

Merge branch 'main' into industry-comparison

c090e6b

DiogoSoaress merged commit ca43bb3 into safe-global:main Aug 20, 2024
3 of 4 checks passed

github-actions bot locked and limited conversation to collaborators Aug 20, 2024

		canvasRef: React.RefObject<HTMLCanvasElement>,
		containerRef: React.RefObject<HTMLDivElement>,

feat: 🎸 Add Industry Comparison Section #427

feat: 🎸 Add Industry Comparison Section #427

Conversation

Malayvasa commented Jul 30, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Malayvasa commented Aug 1, 2024

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

DiogoSoaress left a 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

DiogoSoaress left a comment

Choose a reason for hiding this comment

Malayvasa commented Aug 20, 2024