Skip to content

Conversation

TomCorvus
Copy link
Contributor

Description

Since [email protected], the onLayout prop is no longer available on the Canvas component. We now retrieve the size using a ref on the Canvas.

Fixes: #604

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Note: Users must upgrade react-native-skia to version 2.2.2 or later.

How Has This Been Tested?

I verified that both Polar and Cartesian charts render at the correct size.
To minimize changes, the size is now retrieved using a Canvas ref, and the onLayout logic is preserved inside a useLayoutEffect, as recommended by the RN Skia team:
https://shopify.github.io/react-native-skia/docs/canvas/overview#canvas-size

Checklist

  • I have included a changeset if this change will require a version change to one of the packages
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have run yarn run check:code and all checks pass
  • I have created a changeset for new features, patches, or major changes
  • I have added tests that prove my fix is effective or that my feature works
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Notes

Some ESLint warnings may appear regarding useLayoutEffect dependencies (ref, onLayout).
Since both values are stable, these warnings can be safely ignored using // eslint-disable-next-line react-hooks/exhaustive-deps.

Copy link

changeset-bot bot commented Aug 6, 2025

🦋 Changeset detected

Latest commit: 831e652

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
victory-native Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Aug 6, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
victory-native-xl-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 11, 2025 7:25pm

@zibs
Copy link
Contributor

zibs commented Aug 6, 2025

Ooh, this is interesting, thanks for following up here @TomCorvus -- will take a look at this.

@zibs zibs self-assigned this Aug 7, 2025
@zibs zibs requested review from zibs and Copilot August 7, 2025 15:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the code to work with react-native-skia version 2.2.x by replacing the deprecated onLayout prop on Canvas components with a ref-based approach for measuring Canvas size.

  • Replace onLayout prop with useCanvasRef hook and ref on Canvas components
  • Update onLayout callback signature to accept width/height directly instead of LayoutChangeEvent
  • Upgrade react-native-skia dependency from 2.0.2 to 2.2.2

Reviewed Changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

File Description
package.json Adds packageManager field specification
lib/src/polar/PolarChart.tsx Replaces Canvas onLayout with ref-based measurement using useLayoutEffect
lib/src/cartesian/CartesianChart.tsx Replaces Canvas onLayout with ref-based measurement using useLayoutEffect
example/package.json Upgrades react-native-skia dependency to version 2.2.2

@zibs
Copy link
Contributor

zibs commented Aug 8, 2025

What do you think about an approach like this @TomCorvus? I'm trying to find a way that doesn't require anyone to be on a specific version of react-native-skia as I can, and I think if we just move the onLayout onto the parent's of the canvas, we should be able to have similar results. Should be a pretty simple diff to take for a spin.

The example seems to be okay with this, but let me know what you think!

diff --git a/lib/src/cartesian/CartesianChart.tsx b/lib/src/cartesian/CartesianChart.tsx
index 063797f..3d209af 100644
--- a/lib/src/cartesian/CartesianChart.tsx
+++ b/lib/src/cartesian/CartesianChart.tsx
@@ -649,7 +649,7 @@ function CartesianChartContent<
 
   // Body of the chart.
   const body = (
-    <Canvas style={{ flex: 1 }} onLayout={onLayout}>
+    <Canvas style={{ flex: 1 }}>
       {YAxisComponents}
       {XAxisComponents}
       {FrameComponent}
@@ -689,7 +689,10 @@ function CartesianChartContent<
   }
 
   return (
-    <GestureHandlerRootView style={{ flex: 1, overflow: "hidden" }}>
+    <GestureHandlerRootView
+      style={{ flex: 1, overflow: "hidden" }}
+      onLayout={onLayout}
+    >
       {body}
       <GestureHandler
         config={gestureHandlerConfig}
diff --git a/lib/src/polar/PolarChart.tsx b/lib/src/polar/PolarChart.tsx
index e9327c0..f11a7db 100644
--- a/lib/src/polar/PolarChart.tsx
+++ b/lib/src/polar/PolarChart.tsx
@@ -33,7 +33,7 @@ type PolarChartBaseProps = {
 };
 
 const PolarChartBase = (
-  props: React.PropsWithChildren<PolarChartBaseProps>,
+  props: React.PropsWithChildren<PolarChartBaseProps>
 ) => {
   const {
     containerStyle,
@@ -52,15 +52,14 @@ const PolarChartBase = (
     composed = Gesture.Race(
       composed,
       pinchTransformGesture(transformState),
-      panTransformGesture(transformState),
+      panTransformGesture(transformState)
     );
   }
 
   return (
-    <View style={[styles.baseContainer, containerStyle]}>
+    <View onLayout={onLayout} style={[styles.baseContainer, containerStyle]}>
       <GestureHandlerRootView style={{ flex: 1, overflow: "hidden" }}>
         <Canvas
-          onLayout={onLayout}
           style={StyleSheet.flatten([
             styles.canvasContainer,
             hasMeasuredLayoutSize ? { width, height } : null,
@@ -84,7 +83,7 @@ type PolarChartProps<
   RawData extends Record<string, unknown>,
   LabelKey extends StringKeyOf<InputFields<RawData>>,
   ValueKey extends StringKeyOf<NumericalFields<RawData>>,
-  ColorKey extends StringKeyOf<ColorFields<RawData>>,
+  ColorKey extends StringKeyOf<ColorFields<RawData>>
 > = {
   data: RawData[];
   colorKey: ColorKey;
@@ -98,11 +97,11 @@ export const PolarChart = <
   RawData extends Record<string, unknown>,
   LabelKey extends StringKeyOf<InputFields<RawData>>,
   ValueKey extends StringKeyOf<NumericalFields<RawData>>,
-  ColorKey extends StringKeyOf<ColorFields<RawData>>,
+  ColorKey extends StringKeyOf<ColorFields<RawData>>
 >(
   props: React.PropsWithChildren<
     PolarChartProps<RawData, LabelKey, ValueKey, ColorKey>
-  >,
+  >
 ) => {
   const { data, labelKey, colorKey, valueKey } = props;
 
@@ -116,8 +115,9 @@ export const PolarChart = <
       setHasMeasuredLayoutSize(true);
       setCanvasSize(layout);
     },
-    [],
+    []
   );
 
   return (
     <FiberProvider>

@TomCorvus
Copy link
Contributor Author

@zibs This approach works better for me — it completely avoids locking to a specific Skia version.
I've committed changes accordingly. Thanks!

Copy link
Contributor

@zibs zibs left a comment

Choose a reason for hiding this comment

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

Thank you very much @TomCorvus - great catch.

@zibs zibs merged commit cf27368 into FormidableLabs:main Aug 11, 2025
3 checks passed
@fmd-ci fmd-ci mentioned this pull request Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

<Canvas onLayout={onLayout} /> is not supported on the new architecture
2 participants