Skip to content

Conversation

@DylanVann
Copy link
Contributor

@DylanVann DylanVann commented May 26, 2018

Interpolate function from #10 (comment) with extrapolation handling, examples, and documentation.

Closes #10 .

# Bundle artifact
*.jsbundle

# metro-with-symlinks
Copy link
Contributor Author

@DylanVann DylanVann May 26, 2018

Choose a reason for hiding this comment

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

This (metro-with-symlinks) makes developing while experimenting with the examples a lot easier. With this you can use yarn link.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Thanks so much for working on this. I realise its a WIP but noticed one thing about your code and wanted to let you know

src/derived.js Outdated

if (left === Extrapolate.EXTEND) {
} else if (left === Extrapolate.CLAMP) {
output = max(outputRange[0], output);
Copy link
Member

Choose a reason for hiding this comment

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

not entirely sure but this seems to be only correct if outputRange was sorted from min to max which is not a requirement. We only expect inputRange to be sorted that way.

I think it would be better to do something along these lines both for CLAMP and IDENTITY:

if (left === Extrapolate.EXTEND) {
} else if (left === Extrapolate.CLAMP) {
  output = cond(lessThan(value, inputRange[0]), outputRange[0], output);
} else if (left === Extrapolate.IDENTITY) {
  output = cond(lessThan(value, inputRange[0]), value, output);
}

similarly for "right"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I'll change it 👍

@kmagiera
Copy link
Member

Also @DylanVann I see you have a TODO list here but I think it would make it a bit easier if we could merge parts of this PR before you are done.

As this PR only adds new functionality and does not change any existing behavior I suggest that we do as follows:

  • I'd be ok with merging your example code and interpolate node (from derived.js) as is in the current state (assuming you address my inline comment). You could send it out in a separate PR if you'd like and keep this one to track your progress.
  • I'd like for that other PR to remove changes related to metro-with-symlinks. My main concern is that I want example project to just work w/o some extra steps like running yarn link somewhere. I'm open to discussing it but to hold this discussion in separate PR
  • Input validation is a must but I'm fine with having it added in a follow-up PR
  • What I'd like to have is RN Animated style "interpolate" method added to Animated.Value. This should be rather simple and mainly want to have it for compatibility purposes. Would still recommend "functional" approach in the docs as it is more inline with the rest of the API
  • As for "testing" this project still has no proper "setup" and would like to look into that some time soon. Would appreciate any help but as long as that setup is missing there is no point in writing tests only for the interpolation bit

@DylanVann
Copy link
Contributor Author

@kmagiera I'll remove the metro stuff for another PR and update this to be interpolate + the examples.

What do you have in mind for input validation? Since the nodes are animated I don't know how we can validate that they're monotonically increasing for inputRange. (also I noticed I said outputRange should be monotonically increasing as well, but that isn't true, I'll remove it from the readme).

"start": "node node_modules/react-native/local-cli/cli.js start",
"start": "metro-with-symlinks start",
"test": "jest",
"postinstall": "rm -rf node_modules/react-native-reanimated/{.git,node_modules,Example}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used this with yarn link and it deleted my git repo 😢 , but we can discuss on another PR.

@DylanVann
Copy link
Contributor Author

Done. Yeah I'd also be happy with merging and doing more cleanup/refactoring of the examples in other PRs.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

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

Sorry I couldn't get back to it sooner.

This looks great! Will merge it right away. Just wanted to check with you if you maybe have time to send follow-up PRs with the things we discussed earlier.

@kmagiera
Copy link
Member

kmagiera commented Jun 1, 2018

As for input validation I agree that it may not be possible to validate if animated values are passed. But I believe that more common usecase is to just pass numbers there in which case we could validate. Also regardless on that we can verify that input and output range tables have at least 2 elements and both are of the same length.

@kmagiera kmagiera merged commit b9ba694 into software-mansion:master Jun 1, 2018
@DylanVann
Copy link
Contributor Author

DylanVann commented Jun 1, 2018

@kmagiera I'll do a PR for:

  • Input validation:
    • Length of arrays.
    • Monotonically increasing if we're able to check.
  • Interpolate method.

Would you be opposed to using eslint with standard as well? There are a bunch of unused imports in the code but no checking of them.

@DylanVann DylanVann deleted the feat/interpolate branch June 1, 2018 16:23
@hannojg hannojg mentioned this pull request Aug 2, 2021
3 tasks
MatiPl01 added a commit that referenced this pull request Jan 21, 2025
* Add example app setup files

* Add some utility components and example screen placeholders

* Add missing type export

* Add one of previous examples as a playground example
MatiPl01 added a commit that referenced this pull request Jan 21, 2025
* Add example app setup files

* Add some utility components and example screen placeholders

* Add missing type export

* Add one of previous examples as a playground example
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.

2 participants