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

[MapView] Support for annotation callouts, annotation press, callout presses and pin animation #1247

Closed
wants to merge 4 commits into from

Conversation

dvcrn
Copy link
Contributor

@dvcrn dvcrn commented May 12, 2015

Started from here - #1120. Most functionality for annotations were missing so I started implementing and somehow got caught up until the entire thing was done.

screen shot 2015-05-12 at 10 07 43 pm

2 new events:

  • callout presses (left / right)
  • annotation presses

6 new properties for annotations:

  • hasLeftCallout
  • hasRightCallout
  • onLeftCalloutPress
  • onRightCalloutPress
  • animateDrop
  • id

1 new property for MapView

  • onAnnotationPress

Now the important thing is, that I implemented all of this the way "I would do it". I am not sure this is the 'reacty' way so please let me know my mistakes 😄

The problem is that there is no real way to identify annotations which makes it difficult to distinguish which one got clicked. The idea is to pass a id and whether it has callouts the entire way with the annotation. I had to subclass MKAnnotation for this and implemented a custom RCTPointAnnotation which adds id, hasLeftCallout, hasRightCallout and animateDrop.

I added RCTPointAnnotation and RCTPointAnnotationArray into a new RCTConvert method.

The user is able to overwrite the id on the annotation directly, but if no id is set, I am generating one on componentWillMount or componentWillReceiveProps. This id will also get passed into onAnnotationPress


I am setting the callout accessory always to UIButtonTypeDetailDisclosure. Todo at some point would be to allow custom UIButtons and Images

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 12, 2015
@dvcrn dvcrn changed the title Add support for MapView annotation callouts, annotation press and callout presses [MapView] Add support for annotation callouts, annotation press and callout presses May 14, 2015
@dvcrn dvcrn changed the title [MapView] Add support for annotation callouts, annotation press and callout presses [MapView] Support for annotation callouts, annotation press, callout presses and pin animation May 14, 2015
@vjeux vjeux assigned a2 May 14, 2015
@brentvatne
Copy link
Collaborator

@dvcrn - Nice one! Sorry for the lack of love on this PR - could you possibly squash your commits and rebase against master? Thanks!

@chandu0101
Copy link
Contributor

@dvcrn excellent , this is what i am looking for :)

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 7, 2015

@brentvatne Squashed and rebased current master

@brentvatne
Copy link
Collaborator

@dvcrn - sorry could you rebase again and ping @a2 when it's done? the project is moving so quickly 😄

- added didSelectAnnotationView event to MapView

- only pass annotation data onAnnotationPress

- added support for callouts in annotations and callout click handling

- passing annotation id on annotation click

- added check for MKuserLocation

- added project.pbxproj changes

- fixed flowtype errors

- fix componentWillReceiveProps not updating annotations correctly

- added option to animate pin drop

- fix callouts not showing when no accessory is set

- noone saw that :)

- added a real identifier rather than using the annotation one

- added missing type annotations
@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 16, 2015

@a2 ping 😄

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 17, 2015

I am working a lot with mapview lately and found some more annoyances so why not squeeze them in here as well?

I updated setAnnotations to now delta-update annotations rather than completely deleting all of them and setting them again. Why does it matter? Because this left very ugly jumpy effects on the map where all annotations suddenly disappear and re-appear. It becomes even more prominent when animateDrop from this change here is on, because it will cause all annotations to drop every time when the annotations change.

I tried object comparison (as in [NSArray containsObject:annotation) but had cases where it would still delete the annotation even though it already existed. I assume the object either changed or a new one got generated but my objc skills are not sufficient to know the exact lifecycle of objects. So instead, I decided to use the identifier, since this change will force each annotation to have one. Because of that, there are a few more loops and comparisons than maybe necessary but the results are great and pins don't get re-dropped if they are already there.

Now the problem: Working on this change made me realize that the identifier system I currently have in place is a bit flawed. If the user sets his own ID, the pins will not change. If the user does not do that and relies on the auto-id I am generating, it will re-add a new ID every time the annotation array changes since it will wipe the unique-id of that annotation and generate a new one.

I decided to replace the unique uuid with a hash containing a JSON.stringify representation of the annotation object. I'd love to hear some feedback on this. The good thing about this solution is, that same objects have same hashes and it is no longer random. This allows for the same pin to have the same identifier through the entire app flow, even if the user replaces the object reference with a new one (e.g. on ajax fetch). No more jumpy pins!

The bad thing is, that if 2 pins are identical, the event system could fail and only 1 callback is getting executed. Though this can only happen if literally 2 pins are on the same lat, long with the same title and subtitle which... well... shouldn't happen.

TODO is to base64 encode that hash, but btoa doesn't seem to be available and I didn't want to add a base64 encoder just for that. So right now it is a looong encodeURIComponent string.

@a2
Copy link
Contributor

a2 commented Jun 17, 2015

This PR was imported internally but @nicklockwood had some comments. Nick, can you make those comments here?

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 18, 2015

Thanks for the feedback, I'll try to get the stuff fixed the next days

@brentvatne
Copy link
Collaborator

Awesome 😄 thanks @dvcrn

@dvcrn
Copy link
Contributor Author

dvcrn commented Jun 22, 2015

Ok I think I got them all

@bparadie
Copy link

@dvcrn Hey, I think I know a workaround for the Travis CI failures. Those have nothing to do with your code changes. You have to add this line to the dependencies in package.json:

"babel-core": "5.5.6",

For more details, see #1546 (comment)

@a2
Copy link
Contributor

a2 commented Jun 25, 2015

Merged internally.

@frantic frantic closed this in 99bc08c Jun 26, 2015
@dvcrn dvcrn deleted the on-annotation-press branch July 6, 2015 09:39
ayushjainrksh pushed a commit to MLH-Fellowship/react-native that referenced this pull request Jul 2, 2020
* imp: Snack example for SectionList

* Revert prettier mess
mganandraj pushed a commit to mganandraj/react-native that referenced this pull request Aug 5, 2022
Split rn-tester build into debug and release
ryanlntn pushed a commit to ryanlntn/react-native that referenced this pull request Aug 9, 2022
Split rn-tester build into debug and release
ryanlntn pushed a commit to ryanlntn/react-native that referenced this pull request Aug 9, 2022
…ester

Merge pull request facebook#1247 from rasaha91/split-rn-tester
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants