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

Added support for AnimatedRegion without modifying the AnimatedImplementation.js of react-native #608

Merged

Conversation

IjzerenHein
Copy link
Contributor

This pull request replaces the old one (#592) (was having too much trouble rebasing due to upstream changes). This pull request is rebased on the latest master and adds an additional check to verify that the AnimatedWithChildren class has been correctly obtained.

@lelandrichardson
Copy link
Collaborator

FYI @IjzerenHein , I hadn't really thought to do something like this (get AnimatedWithChildren through the prototype chain) because the choice to not export those classes was made explicitly, and I was trying to figure out a good way to extend this system.

I came up with a bit of a strategy, which I PR'd here: animatedjs/animated#12 If you'd be interested in taking a look.

// longitudeDelta: string,
// }};
const AnimatedWithChildren = Object.getPrototypeOf(Animated.ValueXY);
if (AnimatedWithChildren.name !== 'AnimatedWithChildren') console.error('AnimatedRegion could not obtain AnimatedWithChildren base class');
Copy link
Collaborator

Choose a reason for hiding this comment

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

will this get preserved through a minification pass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great question :)
I'll let you know when I know for sure :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

might not be terrible to just wrap it in if(__DEV__)

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 made a release build but I couldn't get it to trigger the error.
Still, I think you're right and we should stay on the safe side here and wrap it in a DEV

@lelandrichardson
Copy link
Collaborator

I suppose if we added addListener to the prototype of Animated, then we'd be okay just using the instanceof Animated check, or am I missing something?. IIRC from talking to @vjeux addListener could be on the Animated "interface" if we wanted it to be, there just wasn't a need for it at the time.

@vjeux
Copy link

vjeux commented Sep 25, 2016

I don't really care where this lives :) If putting it on the prototype makes it easier for you, feel free to

…it doesn’t break minification/obfuscation builds
@IjzerenHein
Copy link
Contributor Author

@lelandrichardson I added the DEV check 👍

As for the interpolate discussion. I'd love to take a closer look at that but frankly I've got a big deadline coming up I don't have the time nor the need for it (at the moment).
I'll revisit this in a later stage. Ping me in case I forget.

cheers, Hein

@lelandrichardson
Copy link
Collaborator

@IjzerenHein for sure. this is strictly better than how it was before.

This LGTM.

@mjsisley
Copy link

I am unable to get the example running on your PR (after changing Animated.Region to MapView.AnimatedRegion. Running the example project with this PR (and also the simpler example from the readme, results in: `undefined in not an object (evaluating 'this.state.XXX).

Is it possible to add a simple example?

@IjzerenHein
Copy link
Contributor Author

@mjsisley Hmm that's odd. Could you check whether the MapView.AnimatedRegion property contains a valid reference to the class? Could you also include a stacktrace of the error?

So, MapView.AnimatedRegion is basically a direct replacement for Animated.Region, so it should work exactly the same. It does have some restrictions and should be used slightly as Animated.Value (you can't use Animated.spring or Animated.timing but should call .spring and .timing directly on the AnimatedRegion).

Example:

export default class MyView extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      coordinate: new MapView.AnimatedRegion({
        latitude: 5.2443,
        longitude: 6.1223
      })
    }
  }

  render() {
    return <MapView>
      <MapView.Marker.Animated coordinate={this.state.coordinate} />
    </MapView>;
  }

  setCoordinateImmediate() {
    this.state.coordinate.setValue({
      latitude: 7.234,
      longitude: 1.233
    });
  }

  setCoordinateAnimated() {
    this.state.coordinate.timing({
      latitude: 7.234,
      longitude: 1.233,
      duration: 1000
    }).start();
  }
}

@mjsisley
Copy link

The full stack trace when the original example is used with your PR subbing MapView.AnimatedRegion for Animated.Region:

undefined is not an object (evaluating 'this.state.panY') 
onMoveShouldSetPanResponder AnimatedViews.js:292 
executeDispatchesInOrderStopAtTrueImpl EventPluginUtils.js:137 executeDispatchesInOrderStopAtTrue EventPluginUtils.js:153 
setResponderAndExtractTransfer ResponderEventPlugin.js:334 
extractEvents ResponderEventPlugin.js:443 
extractEvents EventPluginHub.js:194 
handleTopLevel ReactEventEmitterMixin.js:28 
<unknown> ReactNativeEventEmitter.js:125 
perform Transaction.js:138 
batchedUpdates ReactDefaultBatchingStrategy.js:63 
batchedUpdates ReactUpdates.js:98 
_receiveRootNodeIDEvent ReactNativeEventEmitter.js:124 
receiveTouches ReactNativeEventEmitter.js:186 
__callFunction MessageQueue.js:229 
<unknown> MessageQueue.js:111 
guard MessageQueue.js:42 
callFunctionReturnFlushedQueue MessageQueue.js:110

It seems the problem is with the usage PanResponder, and thus unrelated to the PR. Just looked closer at the examples and realized that AnimatedViews example is also the only example using the PanResponder... so this is a separate issue.

Thanks for you help!

Plugging the following into the AirMapsExplorer project does work (slight modification of your example for the example project):

import React from 'react';
import {
  StyleSheet,
  View,
  Animated,
} from 'react-native';

import MapView from 'react-native-maps';

class MyView extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      coordinate: new MapView.AnimatedRegion({
        latitude: 5.2443,
        longitude: 6.1223
      })
    }
  }

  render() {
    return <View style={styles.container}>
      <MapView style={styles.map}>
        <MapView.Marker.Animated coordinate={this.state.coordinate} />
      </MapView>
    </View>;
  }

  setCoordinateImmediate() {
    this.state.coordinate.setValue({
      latitude: 7.234,
      longitude: 1.233
    });
  }

  setCoordinateAnimated() {
    this.state.coordinate.timing({
      latitude: 7.234,
      longitude: 1.233,
      duration: 1000
    }).start();
  }
}

const styles = StyleSheet.create({
  container: {
    ...StyleSheet.absoluteFillObject,
  },
  map: {
    backgroundColor: 'transparent',
    ...StyleSheet.absoluteFillObject,
  },
});

module.exports = MyView;

@IjzerenHein
Copy link
Contributor Author

Alright, great to hear that has been resolved @mjsisley 👍

if (__DEV__) {
if (AnimatedWithChildren.name !== 'AnimatedWithChildren') console.error('AnimatedRegion could not obtain AnimatedWithChildren base class');
}
// const __Animated = Object.getPrototypeOf(AnimatedWithChildren);

Choose a reason for hiding this comment

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

Remove these comments?

@spikebrehm
Copy link

Awesome! Thanks for your hard work! 🍻

@spikebrehm spikebrehm merged commit 481d75b into react-native-maps:master Sep 28, 2016
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.

5 participants