↕️ useCanvasSize() #380
↕️ useCanvasSize() #380
Conversation
package/src/renderer/Canvas.tsx
Outdated
| const canvasCtx = useValue<{ width: number; height: number }>({ | ||
| width: -1, | ||
| height: -1, | ||
| }); |
There was a problem hiding this comment.
Here it will be -1 always on the first render but I'm not sure how we could deal with this.
There was a problem hiding this comment.
That is the only way to go I think.
| const root = useMemo( | ||
| () => skiaReconciler.createContainer(container, 0, false, null), | ||
| [container] | ||
| ); | ||
| // Render effect | ||
| useEffect(() => { | ||
| render( | ||
| <CanvasContext.Provider value={canvasCtx.current}> | ||
| <CanvasContext.Provider value={canvasCtx}> |
There was a problem hiding this comment.
This doesn't seem to be play well with hot reload, could this be improved?
There was a problem hiding this comment.
How does the reload problem materialise itself? It looks correct since the effect is dependent on the canvasCtx?
package/src/renderer/Canvas.tsx
Outdated
| @@ -93,6 +72,7 @@ const defaultFontMgr = Skia.FontMgr.RefDefault(); | |||
|
|
|||
| export const Canvas = forwardRef<SkiaView, CanvasProps>( | |||
| ({ children, style, debug, mode, onTouch, fontMgr }, forwardedRef) => { | |||
| const canvasCtx = useValue({ width: -1, height: -1 }); | |||
There was a problem hiding this comment.
default value should be 0.
|
I updated the default size and I can tell that the fast refresh works great (the issue was when updating the Canvas.tsx file) |
package/src/renderer/Canvas.tsx
Outdated
| @@ -93,6 +72,7 @@ const defaultFontMgr = Skia.FontMgr.RefDefault(); | |||
|
|
|||
| export const Canvas = forwardRef<SkiaView, CanvasProps>( | |||
| ({ children, style, debug, mode, onTouch, fontMgr }, forwardedRef) => { | |||
| const canvasCtx = useValue({ width: -1, height: -1 }); | |||
There was a problem hiding this comment.
Could this be a regular ref? The only reason for using a value would be that we want to be able to listen to changes on the value and this kind of does not make sense for the context?
There was a problem hiding this comment.
yes that's correct. I wanted to keep the types of the public API same but let me check if using a ref instead still keep the public types safe.
| height !== canvasCtx.current.height | ||
| ) { | ||
| canvasCtx.current = { width, height }; | ||
| } |
There was a problem hiding this comment.
I'm thinking it might be possible to include more of the drawing context - I saw your comment about wanting to do so. Is there a specific reason for not doing so?
There was a problem hiding this comment.
The idea was that useDerived() that depends on the canvasCtx value should only be run if we modify the width/height. But the expression is always re-evaluated on redraw? I will double check.
There was a problem hiding this comment.
Just checked and this does save some unnecessary JS evaluation. but of course let's keep it in mind (as well as offering a way to access the ref)
| */ | ||
| onDraw?: RNSkiaDrawCallback; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Any specific reason for moving these types into the SkiaView file?
There was a problem hiding this comment.
Having the Props and the components together felt clearer which is which (NativeSkiaViewProps vs SkiaViewProps). I see also that NativeSkiaViewProps and requireComponent also live in separate file.
fixes #352
Exposes the canvas size as a Skia Value.
It also exposes the onLayout prop which could be useful.
Originally I wanted to expose the whole
DrawingInfovia this hook but realized that it would update more than usually needed.