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

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
@dvcrn
Contributor

dvcrn commented Jul 8, 2015

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.

screen shot 2015-07-08 at 8 02 59 pm

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 dvcrn changed the title from Mapview pin refactor to [MapView] add image support for annotations, refactored config Jul 8, 2015

@dvcrn dvcrn changed the title from [MapView] add image support for annotations, refactored config to [MapView] add image support for annotations callouts, refactored config Jul 8, 2015

@dvcrn dvcrn changed the title from [MapView] add image support for annotations callouts, refactored config to [MapView] add image support for annotation callouts, refactored config Jul 8, 2015

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 9, 2015

Contributor

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

Contributor

dvcrn commented Jul 9, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 15, 2015

Contributor

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?

Contributor

sahrens commented Jul 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 15, 2015

Contributor

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

Contributor

sahrens commented Jul 15, 2015

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

@sahrens

This comment has been minimized.

Show comment
Hide comment
@sahrens

sahrens Jul 15, 2015

Contributor

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

Contributor

sahrens commented Jul 15, 2015

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

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 16, 2015

Contributor

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.

Contributor

dvcrn commented Jul 16, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 22, 2015

Contributor

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

Contributor

dvcrn commented Jul 22, 2015

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

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 29, 2015

Contributor

ping @a2. Anything else needed for this? 😄

Contributor

dvcrn commented Jul 29, 2015

ping @a2. Anything else needed for this? 😄

@a2

This comment has been minimized.

Show comment
Hide comment
@a2

a2 Jul 29, 2015

Contributor
Contributor

a2 commented Jul 29, 2015

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@a2

This comment has been minimized.

Show comment
Hide comment
@a2

a2 Jul 29, 2015

Contributor

@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.

Contributor

a2 commented Jul 29, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jul 29, 2015

Contributor

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?

Contributor

vjeux commented Jul 29, 2015

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

This comment has been minimized.

Show comment
Hide comment
@a2

a2 Jul 29, 2015

Contributor

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

Contributor

a2 commented Jul 29, 2015

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

@a2 a2 assigned vjeux and unassigned a2 Jul 30, 2015

@a2

This comment has been minimized.

Show comment
Hide comment
@a2

a2 Jul 30, 2015

Contributor

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

Contributor

a2 commented Jul 30, 2015

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

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 30, 2015

Contributor

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?

Contributor

dvcrn commented Jul 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jul 30, 2015

Contributor

@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?

Contributor

nicklockwood commented Jul 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Jul 30, 2015

Contributor

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

Contributor

vjeux commented Jul 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jul 31, 2015

Contributor

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?

Contributor

dvcrn commented Jul 31, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Aug 14, 2015

Contributor

Ping. Is there anything I can help with here?

Contributor

dvcrn commented Aug 14, 2015

Ping. Is there anything I can help with here?

@a2

This comment has been minimized.

Show comment
Hide comment
@a2

a2 Aug 14, 2015

Contributor

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

Contributor

a2 commented Aug 14, 2015

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

React/Views/RCTMapManager.m
+ if ([[annotation valueForKey:typeSelector] integerValue] == RCTPointAnnotationTypeImage) {
+ [annotationView setValue:[self generateCalloutImageAccessory:[annotation valueForKey:configSelector]] forKey:accessoryViewSelector];
+ }
+ }

This comment has been minimized.

@machard

machard Aug 26, 2015

Contributor

this block would be better in didSelectAnnotationView
When you have a lot of pin, all images are created AND downloaded when the map is displayed. It can fill your memory and crash the app!
Moving it to didSelectAnnotation will create and load image only when the pin is tapped.

@machard

machard Aug 26, 2015

Contributor

this block would be better in didSelectAnnotationView
When you have a lot of pin, all images are created AND downloaded when the map is displayed. It can fill your memory and crash the app!
Moving it to didSelectAnnotation will create and load image only when the pin is tapped.

This comment has been minimized.

@dvcrn

dvcrn Aug 28, 2015

Contributor

@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

dvcrn Aug 28, 2015

Contributor

@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

This comment has been minimized.

@machard

machard Aug 28, 2015

Contributor

it was in case someone like me take the code and has the the same crash problem i had :)

@machard

machard Aug 28, 2015

Contributor

it was in case someone like me take the code and has the the same crash problem i had :)

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Sep 14, 2015

Contributor

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

Contributor

dvcrn commented Sep 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@edcs

edcs Sep 30, 2015

Contributor

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

Contributor

edcs commented Sep 30, 2015

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

@vjeux vjeux removed their assignment Oct 16, 2015

mkonicek and others added some commits Oct 23, 2015

Fixed dev mode override to work with unminified JS
Summary: public

The dev mode override feature was built with the assumption that bunlded JS would be minified, and broke with unminified JS. This fixes that by using a more robust regex-based search.

Reviewed By: tadeuzagallo

Differential Revision: D2581240

fb-gh-sync-id: 4d4b45eb8573ceb956b7259550d80a9807f83d59
refactored annotation config, added support for images instead of but…
…tons, further prepared for more types of accessories

dvcrn added some commits Jul 14, 2015

offloaded image view generation into separate function
- offloaded image generation for callouts into a separate function to
reduce code size by a good chunk.
- Fixed default images not getting used correctly
- Added status code check for remote images
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Nov 6, 2015

@dvcrn updated the pull request.

@dvcrn updated the pull request.

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Nov 6, 2015

Contributor

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?

Contributor

dvcrn commented Nov 6, 2015

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

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Dec 22, 2015

Collaborator

Any updates on this?

Collaborator

satya164 commented Dec 22, 2015

Any updates on this?

@bestander bestander self-assigned this Jan 16, 2016

@satya164

This comment has been minimized.

Show comment
Hide comment
@satya164

satya164 Jan 26, 2016

Collaborator

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

Collaborator

satya164 commented Jan 26, 2016

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

@satya164 satya164 closed this Jan 26, 2016

@nicklockwood

This comment has been minimized.

Show comment
Hide comment
@nicklockwood

nicklockwood Jan 26, 2016

Contributor

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

Contributor

nicklockwood commented Jan 26, 2016

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

@dvcrn

This comment has been minimized.

Show comment
Hide comment
@dvcrn

dvcrn Jan 27, 2016

Contributor

yup - not necessary anymore

Contributor

dvcrn commented Jan 27, 2016

yup - not necessary anymore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment