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: Add interpolateColors function #777

Merged

Conversation

jtomaszewski
Copy link
Contributor

@jtomaszewski jtomaszewski commented May 5, 2020

Description

Lets users animate colors with react-native-reanimated without the need of additional helper functions like this one or this one.

Also will help avoid people from running into a problem "why does this color doesn't animate? :(" like I did today in the morning in one of my projects. :)

Fixes #181 .

@jakub-gonet
Copy link
Member

jakub-gonet commented May 5, 2020

Thanks for your contribution!
This also fixes #262.
EDIT: it doesn't 🙂

I wonder if we should include it directly in interpolate wrapping it or leave as a helper

@jtomaszewski
Copy link
Contributor Author

jtomaszewski commented May 5, 2020

I wonder if we should include it directly in interpolate wrapping it or leave as a helper

Maybe it could be included directly in interpolate, but then the interpolate would need to be able to distinguish between a color output and non-color output, right? and so we'd need to edit the Animated.color (or create a new method for that) to create some kind of a "color class"?

I was thinking about it, but then decided: doing that is definitely more work; maybe needs to be discussed first; and still, I think it can be done later on as improvement. (And doing that improvement wouldn't break the interpolateColors helper as it would still totally work.)

For now I think merging this PR will be very helpful, as it'll make animating colors way easier, and also provide the docs describing how to do it. ( Currently there are none ;P )

Let me know if you want me to do some changes to the PR if anything blocks merging it, I'm happy to help.

@jtomaszewski
Copy link
Contributor Author

Also, it does fix #262 I think, at least the demo - you can see it being fixed after using the helper here: https://snack.expo.io/@jack-ailo/reanimatedcolorbugexample .

@jakub-gonet
Copy link
Member

the interpolate would need to be able to distinguish between a color output and non-color output, right? and so we'd need to edit the Animated.color (or create a new method for that) to create some kind of a "color class"?

You're right, yes. I was convinced that color is a node but it's just a few bitshifts glued together.

Also, it does fix #262 (...)

That's also true, I mistook it for another issue with interpolating colors as strings e.g. from "#fff" to "#000" (which can also be added in some followup)

I think a minor update to docs would be awesome:

  • crossreferencing interpolateColors in the interpolate and color node, so people can see that it exists
  • adding info about [0, 255] range for colors (most users will know about that, but it won't hurt).

After those changes, this will be ready for merging.

@jtomaszewski
Copy link
Contributor Author

Updates to docs added ;)

Copy link
Member

@jakub-gonet jakub-gonet left a comment

Choose a reason for hiding this comment

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

🎉

@wcandillon
Copy link
Contributor

Not sure if this helps but we have implemented such functions in redash: https://github.com/wcandillon/react-native-redash/blob/master/packages/core/src/Colors.ts
maybe it can help to make the API a little bit more user friendly? For instance to pass colors as strings.

@jtomaszewski
Copy link
Contributor Author

I didn't know that react-native has such function as processColor built in. I was afraid that parsing the colors as strings would require including a dependency, but I can see react-native is enough. I'll improve it in a minute. Thanks @wcandillon !

@jtomaszewski
Copy link
Contributor Author

Actually the interpolateColors wasn't exported at all here. I added an example and fixed all the bugs. Now this PR is 100% merge ready IMHO.

@wcandillon
Copy link
Contributor

Just had a look at the diff and it looks good :)

@jakub-gonet jakub-gonet merged commit 94850e6 into software-mansion:master May 22, 2020
@ekatzenstein
Copy link

interpolateColors is not exported with the v1.9.0

@jakub-gonet
Copy link
Member

It's not, it'll be included in the next version.

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.

Color node documentation fixes
5 participants