search

margelo / react-native-graph

📈 Beautiful, high-performance Graphs and Charts for React Native built with Skia
https://margelo.io
MIT License
2.08k stars 118 forks source link

added a multiline graph example #116

Closed anshugarg401 closed 1 week ago

sidferreira commented 2 weeks ago

@anshugarg401 can you explain how this works? You have a lot of new code, so it is tricky to identify exactly what you changed.

chrispader commented 2 weeks ago

Yea thanks @sidferreira for asking. I'd also be curious about the specifics of the implementation

@anshugarg401 if you can provide a little more detail about your implementation we can try to bake this into a new feature in the future! 🚀 🙌

sidferreira commented 2 weeks ago

I had a verybold version where I would allow charts to be on top of each other and use external shared values.

I will try to reproduce it.

gatedude commented 2 weeks ago

@anshugarg401 can you explain how this works? You have a lot of new code, so it is tricky to identify exactly what you changed.

So a request was made on Warpacster to create an example of a multi line graph and solve this GitHub issue "https://github.com/margelo/react-native-graph/issues/22"

Since there was not a fully functioning multi line graph, a request was made to create one and get it merged by the margelo team.

17303139287944976_original.png

Above is the image of how the graph should look like; but the request was that it should be a fully functioning multi line graph just like these other examples: "https://github.com/margelo/react-native-graph/tree/main/example"

The request is that the PR is supposed to cover the full width of react-native-graph features documented in the configuration https://github.com/margelo/react-native-graph/?tab=readme-ov-file#configuration

So, this is what the graph looks like, after everything, here's the video: https://github.com/user-attachments/assets/5a1dabfb-c661-4f2f-a4ea-12c36628a5cf

So, that's why I need the PR to be merged.

gatedude commented 2 weeks ago

@sidferreira please can you merge this?

sidferreira commented 2 weeks ago

@sidferreira please can you merge this?

Merge what?

gatedude commented 2 weeks ago

@sidferreira you asked @anshugarg401 to explain how his code works, he submitted a PR

https://github.com/margelo/react-native-graph/pull/116#issuecomment-2448120679

sidferreira commented 2 weeks ago

@sidferreira you asked @anshugarg401 to explain how his code works, he submitted a PR

https://github.com/margelo/react-native-graph/pull/116#issuecomment-2448120679

I'm not a maintainer, I was curious about the solution

gatedude commented 2 weeks ago

@sidferreira you asked @anshugarg401 to explain how his code works, he submitted a PR

https://github.com/margelo/react-native-graph/pull/116#issuecomment-2448120679

I'm not a maintainer, I was curious about the solution

@sidferreira ok

anshugarg401 commented 2 weeks ago

I change the animated graph and passed the x and y co-ordinates from the usePanGesture hook globally to the component and made a custom pointer with react native skia. Also added a component that iterate over the all the graphs and display there values in the hover box and the overlapping graph lines. The box value is calculated according to the common x co-ordinates of the multiple graphs. Also added XCo-ordinate and YCo-odinate components to display the x and y axis of the graph whose path is passed as argument in the component.

gatedude commented 2 weeks ago

@sidferreira here's how the code works: https://github.com/margelo/react-native-graph/pull/116#issuecomment-2451615835

@mrousavy

sidferreira commented 2 weeks ago

@sidferreira here's how the code works: https://github.com/margelo/react-native-graph/pull/116#issuecomment-2451615835

@mrousavy

Oh, so overlappinggraphs is the secret...

gatedude commented 2 weeks ago

@sidferreira here's how the code works: https://github.com/margelo/react-native-graph/pull/116#issuecomment-2451615835

@mrousavy

Oh, so overlappinggraphs is the secret...

@sidferreira yeah that's how it works;

gatedude commented 2 weeks ago

I change the animated graph and passed the x and y co-ordinates from the usePanGesture hook globally to the component and made a custom pointer with react native skia. Also added a component that iterate over the all the graphs and display there values in the hover box and the overlapping graph lines. The box value is calculated according to the common x co-ordinates of the multiple graphs. Also added XCo-ordinate and YCo-odinate components to display the x and y axis of the graph whose path is passed as argument in the component.

@mrousavy is it possible to get the PR merged? Here's how the code works!

sidferreira commented 2 weeks ago

Is there a way to cut out some of the extra files?

anshugarg401 commented 2 weeks ago

@sidferreira yes, I will remove all the extra code and make it make a new PR.

sidferreira commented 2 weeks ago

@sidferreira yes, I will remove all the extra code and make it make a new PR.

Keep the same pr, will be better

chrispader commented 2 weeks ago

yes, I will remove all the extra code and make it make a new PR.

Lmk once there is a new lean PR, i'll take a look at it once it's up 🙌

gatedude commented 2 weeks ago

@chrispader are you part of the margelo team?

anshugarg401 commented 1 week ago

@chrispader you can checkout the new changes remove some extra code.

gatedude commented 1 week ago

@chrispader you can checkout the new changes remove some extra code.

@chrispader please can you review the PR and merge it? Please I need it merge so I can move forward! Also, I need the PR merged so I can move forward with @anshugarg401

chrispader commented 1 week ago

@chrispader please can you review the PR and merge it? Please I need it merge so I can move forward! Also, I need the PR merged so I can move forward with @anshugarg401

i probably won't get to this very soon, but if you need this feature urgently you can easily patch the package in your app :)

gatedude commented 1 week ago

@chrispader please can you review the PR and merge it? Please I need it merge so I can move forward! Also, I need the PR merged so I can move forward with @anshugarg401

i probably won't get to this very soon, but if you need this feature urgently you can easily patch the package in your app :)

@chrispader 'm not talking about that fam; a Bounty was posted on Farcaster to create and merge a PR in the react native graph.

So, @anshugarg401 have created the PR and I need it merged by the margelo team. It's been weeks now; and if it doesn't get merged by you I don't get paid for the Bounty that's why I need it merged by the margelo team!

The multi link graph has been created just like the instructions; a PR has been submitted by @anshugarg401 so for me to get paid for the Bounty I need the PR merged, if not I don't get paid.

The finished work is supposed to look like this image which it is Screenshot_20241104-012852.jpg

I'm sorry for the Bother but I really need the PR merged or I don't get paid;

How long will it take for it to be merged please?

Screenshot_20241104-012350.jpg

mrousavy commented 1 week ago

With all due respect, this PR will never get merged as it is right now.

Instead of trying to add a new feature, you simply created a new folder in the root folder of the repo, dumped some files into it (it looks like an Expo app with NativeWind installed??), and overlapped some Graphs. You also didn't make any efforts to explain what this actually does, the PR description is just blank.

There is not a single change to the actual react-native-graph package, so this is merely a code example. This will not make it into react-native-graph. Sorry.

gatedude commented 1 week ago

With all due respect, this PR will never get merged as it is right now.

Instead of trying to add a new feature, you simply created a new folder in the root folder of the repo, dumped some files into it (it looks like an Expo app with NativeWind installed??), and overlapped some Graphs. You also didn't make any efforts to explain what this actually does, the PR description is just blank.

There is not a single change to the actual react-native-graph package, so this is merely a code example. This will not make it into react-native-graph. Sorry.

@mrousavy ok Fam, I'II make the necessary adjustments and submit a new PR

gatedude commented 1 week ago

@mrousavy yes I really made a mistake and did everything the wrong way.

I'II adjust everything and create a new PR!

But please, if I may ask; to do this the right way, how should this be done? I'd appreciate if you could just tell me how I should do this. I'm still learning you see, everyone is.