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: update react-native-skia to 0.1.136 #31

Merged
merged 4 commits into from
Jul 25, 2022

Conversation

akinncar
Copy link
Contributor

@akinncar akinncar commented Jul 17, 2022

Upgrades:

  • react-native-skia
  • react-native-reanimated
  • react-native-gesture-handler

Changes:

Comments

I can't solve this types validation, seems to work for me as expected with @ts-ignore but please help me to solve it, I don't know how this works

Type 'SkiaValue<SkPath | null>' is not assignable to type 'PathDef | SkiaValue<PathDef>'.
  Type 'SkiaValue<SkPath | null>' is not assignable to type 'SkiaValue<PathDef>'.
    Type 'SkPath | null' is not assignable to type 'PathDef'.
      Type 'null' is not assignable to type 'PathDef'.ts(2322)

https://github.com/margelo/react-native-graph/compare/main...akinncar:feat/update-skia?expand=1#diff-cfb3f852e9829282d769b73218a60d92f55779488387a3333b1c5717365419e9R246

Copy link
Member

@mrousavy mrousavy left a comment

Choose a reason for hiding this comment

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

Thanks Akinn! PR looks good to me, just 2 minor changes for the from value that can't be null

@@ -89,11 +89,11 @@ export function AnimatedLineGraph({
})

const previous = paths.current
let from: SkPath = previous.to ?? straightLine
let from: SkPath | null = previous.to ?? straightLine
Copy link
Member

Choose a reason for hiding this comment

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

straightLine can never be null, so this can be removed

Copy link
Contributor Author

@akinncar akinncar Jul 18, 2022

Choose a reason for hiding this comment

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

interpolate now can return null
interpolate type: (method) SkPath.interpolate(end: SkPath, weight: number): SkPath | null
so when you assign interpolate result to from, it can be null
https://github.com/margelo/react-native-graph/pull/31/files/00840df3ff16404aea00355f7092afada7c35c2d#diff-cfb3f852e9829282d769b73218a60d92f55779488387a3333b1c5717365419e9R94

Shopify/react-native-skia#567

Copy link
Member

Choose a reason for hiding this comment

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

But straightLine is not using interpolate?

Copy link
Contributor Author

@akinncar akinncar Jul 19, 2022

Choose a reason for hiding this comment

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

on declaration, from doesn't need to be SkPath | null, but on second assignment, when we assign interpolate to from

from = from.interpolate(previous.from, interpolateProgress.current)

it can be null
image
image
a possible solution is to create another variable but I don't see any difference

Copy link
Member

Choose a reason for hiding this comment

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

I see, hmm this is a bit weird. We could do

from = from.interpolate(previous.from, interpolateProgress.current) ?? from

But that's weird either way. I got your point now

src/AnimatedLineGraph.tsx Outdated Show resolved Hide resolved
@mrousavy
Copy link
Member

Does the point still move correctly with this PR?

I'm seeing this:

Screen.Recording.2022-07-20.at.11.59.37.mov

@gtokman
Copy link
Contributor

gtokman commented Jul 23, 2022

@mrousavy Someone filed an issue for that here #26 (comment)

@akinncar
Copy link
Contributor Author

akinncar commented Jul 24, 2022

Does the point still move correctly with this PR?

Fixed on latest commit

but a comment about it: on the first press on the graph, the circle starts in last position, so if is your first press after opening the app, the point will show on 0/0
It's not related to this PR since it's happening on main branch as well
https://user-images.githubusercontent.com/42688281/180664840-8ae6c55c-6523-4f30-abe7-fdb596c3e109.mp4

UPDATE:

Fixed here #32

@mrousavy
Copy link
Member

Awesome, thank you Akinn! ❤️

@mrousavy mrousavy merged commit dffaa4b into margelo:main Jul 25, 2022
@akinncar akinncar deleted the feat/update-skia branch July 25, 2022 11:34
@HasanBingolbali
Copy link

is it now fully compatible with react-native-Skia 0.1.136? @mrousavy @akinncar

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.

4 participants