[MapView] add image support for annotation callouts, refactored config

[dvcrn]

Addition / refactor to #1247

I realized while using the new callouts that support for images is still missing and that more callout types might get added in the future which is a bit awkward with the old config method. Refactored the code a bit and added image support to make sure there won't be bigger issues later on.

Example:

 leftCallout: {
type: "button",
onPress: function () {
console.info("Test left");
}
},
rightCallout: {
type: "image",
config: {
image: require("image!myIcon")
// Or
image: "http://www.google.com/icon.png"
// Will use the local defaultImage until the web resource has been loaded
defaultImage: require("image!myIcon") }
}

Difference from the last version

hasRightCallout: true,
onRightCalloutPress: function() {}

becomes to

 rightCallout: {
type: "button",
onPress: function () {}
}

• Moved left and right into their own config objects
• added type for allowing to pick the accessory type (currently button and image)
• moved on...CalloutPress into new config under onPress
• added config sub object for passing additional parameters to the accessory. Currently supported are image when type is image, and defaultImage when image is not a local ressource

[dvcrn]

Hmm, looks like I forgot about support for normal urls that are not project resources. I'll try to add that in the next days

[sahrens]

I realize this is just an extension of the style from #1247, but what do you think about making the callouts react components? Then we could support arbitrary callouts instead of just images and buttons. The way we typically do that is to pass the special views as children, then in native insertReactSubview we would grab the callout views and set them as the accessory views. You can differentiate the callout views from normal children/subviews by passing the tag IDs of the callouts as a prop to the parent MapView.

I know that's a lot of work, so we can potentially keep this approach for now and change the API/approach later.

Also, can you remove all unrelated changes to babel, podspec, etc?

[sahrens]

Passing to @a2 who handled the last one, and cc @nicklockwood and @vjeux for thoughts on API.

[sahrens]

Also, can you add usage of this stuff to the map example?

[dvcrn]

Okay finally got the code in a state I'm happy with.

I think using react components is a really cool idea and definitely something to add in the long term. When I started I didn't know how to implement this kind of behavior so went with the simpler solution to extend the API.

I'll edit the map example when I'm back home later the day.

[dvcrn]

Okay, cleaned the commits up and removed stuff that shouldn't be there. Also added some simple usage to MapViewExample.js

[dvcrn]

ping @a2. Anything else needed for this? 😄

[a2]

@facebook-github-bot import

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/102516373428940/int_phab to review.

[a2]

@vjeux Can you look at the JavaScript API for this? I think we could DRY up the duplicate code for the two shapes, and I'm not sure if the inner "config" dictionary is necessary.

[vjeux]

I'm also with the opinion of @sahrens, at this point we're adding a lot more configurations to the popup and instead of hardcoding all those, it would be better to let the user render an arbitrary React component inside. This way we don't need to add a lot of complexity to that component that's not going to match all the use cases and instead provide a generic way to extend it. What do you think?

[a2]

That sounds good. Better to just put in arbitrary components that the user can specify.

[a2]

@vjeux What do we want to do with this PR then?

[dvcrn]

I'll try to take a look at how to pass a react component to the annotation once I'm a bit more free on time. Do you guys have a class/component somewhere where this has already been done for reference?

[nicklockwood]

@dvcrn this is actually not a simple problem, and it's something I've been investigating. The problem is that unless a (non-root) React component is a direct descendant of another component, it's won't get processed by the resolver or layout system. Ideally it would work like this:

_getAnnotations(region) {
return [{
longitude: region.longitude,
latitude: region.latitude,
view: <MyCalloutView/>
}];
},
<MapView annotations={this._getAnnotations}/>

But due to the aforementioned limitations, I think it would actually have to be more like this:

class MyAnnotation extends MapAnnotation {
render () {
return <SomeView>
}
}
<MapView>
<MyAnnotation longitude={...} latitude={...}/>
<MyAnnotation longitude={...} latitude={...}/>
<MyAnnotation longitude={...} latitude={...}/>
</MapView >

i.e. the whole annotation would need to be a component in its own right, and a child of the MapView, otherwise it won't get processed correctly.

@vjeux, does that sound right? Or am I missing something? Maybe there's a way to hide this implementation detail?

[vjeux]

Yeah this is annoying, I'm pretty sure that it hasn't been designed that way but it is just the result of the current way it is implemented and can be fixed

[dvcrn]

I see, thanks for the explanation. Besides that, is this PR in a usable state until that kind of functionality can be implemented or should we directly try to add the annotation view as a react component?
If so, is there anything I can help with to speed things up a bit?

[dvcrn]

Ping. Is there anything I can help with here?

[a2]

I think we should close this until we have proper support for components in callouts. @nicklockwood and @vjeux, thoughts?

[dvcrn]

@machard good point and thanks for the comment. But since this PR is 'on hold', I am not sure whether it's worth changing the code now or just wait for react components in callouts

[dvcrn]

I read a bit through the code and found that TabBarIOS (http://facebook.github.io/react-native/docs/tabbarios.html#content) is implemented very similar and a good reference in how this could look later on.

To clarify, is

<MapView>
<MyAnnotation longitude={...} latitude={...}/>
<MyAnnotation longitude={...} latitude={...}/>
<MyAnnotation longitude={...} latitude={...}/>
</MapView >

the behaviour we want for this? Not that I start and waste my time with this :p

[edcs]

That sort of thing would be ideal for me - I've had a go at implementing it myself but I got a bit stuck.

[facebook-github-bot]

@dvcrn updated the pull request.

[dvcrn]

no more silent updates, huh? 😄
Just rebased 0.14.0 since I'm using this code in some of my apps until a new solution is ready.

Should I close this PR?

[satya164]

Any updates on this?

[satya164]

Closing since no activity on the PR. let's re-open if you wanna work on it again.

[nicklockwood]

Custom callouts views are supported now on iOS, so this PR is now moot.

[dvcrn]

yup - not necessary anymore