-
Notifications
You must be signed in to change notification settings - Fork 120
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
Improve behavior for animated graph with gesture #14
base: main
Are you sure you want to change the base?
Conversation
Hey! Thanks for your PR!
I don't really understand this, could you maybe elaborate a bit? Do you have a screenshot/GIF to compare before/after? |
() => x.value, | ||
(fingerX) => { | ||
runOnJS(setFingerX)(fingerX) | ||
() => [x.value, isActive.value], | ||
([fingerX, isActiveValue]) => { | ||
if (isActiveValue) { | ||
runOnJS(setFingerX)(fingerX as number) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change? Afaik it worked fine before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes with the idea of the graph highlight & the initialIndex. On the initial render, the setFingerX function is executed, but as there is no gesture, the highlight path starts at 0.
Hello Sorry for the misunderstanding. So basically, this is the look we have when the graph renders initially on the current version: With the The The initial goal was to be able to have the opaque blue part going all the way to the end of the graph. But i thought giving more flexibility to put it until a certain index might be better. Let me know if that is more clear for you |
Got it, but I can't think of a use-case for this feature atm. - why would you want to have the path hard-stop at a given point? This whole thing is built as an interactive gesture for the user |
Taking the coinbase app as a reference, the graph is fully highlighted in the initial render. Maybe the case of highlighting the graph partially is a bad idea, but i dont think its a bad idea to highlight the whole graph |
Hello @mrousavy have you had a chance to look at the latest changes ? Thank you 🙂 |
@mrousavy This is a really great idea when visualizing past and forecasted data! Can this PR be approved? I'd like to use this feature. |
Animated graph with gestures enabled had some improvements in my opinion.
This PR adds the following:
initialIndex
prop. This will basically give an initial position to the circle. The old behavior was not really appropriate: the whole graph had a translucent color. With this prop, this gives the possibility to highlight the whole graph.resetPositionOnRelease
prop. Passingtrue
as a value will keep the old behavior, while passingfalse
will keep the circle at the last position where the gesture was released.