Skip to content
This repository has been archived by the owner on May 9, 2018. It is now read-only.

Why not implementing SMCalloutView's delegate method calloutViewClicked in RMMapView? #422

Open
mohpor opened this issue Mar 18, 2014 · 14 comments

Comments

@mohpor
Copy link

mohpor commented Mar 18, 2014

It seems kindda strange that RMMapView _doesn't_ implement SMCalloutViewDelegate's method calloutViewClicked: and pass it to RMMapViewDelegate!

Anyone with a need to handle accessory buttons' tap, might be interested in the callout taps too, it is in place you just need to re-delegate it!

and while we are at it, I guess it seems like a logical request to make _currentCallout public (a property), this way the developer has access to the callout and can handle anything he wants, including adding custom views for the title and all.
I know it might make RMMapView tightly coupled with SMCalloutView but we got to make a hard decision and decide and use SMCalloutView as a core component and rely on it.

@incanus
Copy link
Contributor

incanus commented Mar 18, 2014

On the delegate front, we took the MapKit-like approach of exposing -[RMMapViewDelegate tapOnCalloutAccessoryControl:forAnnotation:onMap:] instead of the whole callout.

https://github.com/mapbox/mapbox-ios-sdk/blob/3223e01e0092ada7c32bf1df8f091b4b6e5f3972/MapView/Map/RMMapViewDelegate.h#L168-L176

Exposing _currentCallout could be interesting, since we're not really doing anything special to it after it is presented other than passing along basic properties. Pull request? ;-)

@mohpor
Copy link
Author

mohpor commented Mar 18, 2014

About the callout delegate, I can see that you've added tap recognizer to accessory views and ignored the only tap delegate provided by SMCalloutView. In my project I need to default the touch on the whole callout todo  something like my right accessory view, but i have no way of doing that without the said delegate method.

And about the exposing calloutview, i have no idea how to request a pull! Maybe if you point me to somewhere I can learn how to request a pull on github?

Sent from Mailbox for iPad

On Tue, Mar 18, 2014 at 8:18 PM, Justin R. Miller
notifications@github.com wrote:

On the delegate front, we took the MapKit-like approach of exposing -[RMMapViewDelegate tapOnCalloutAccessoryControl:forAnnotation:onMap:] instead of the whole callout.
https://github.com/mapbox/mapbox-ios-sdk/blob/3223e01e0092ada7c32bf1df8f091b4b6e5f3972/MapView/Map/RMMapViewDelegate.h#L168-L176

Exposing _currentCallout could be interesting, since we're not really doing anything special to it after it is presented other than passing along basic properties. Pull request? ;-)

Reply to this email directly or view it on GitHub:
#422 (comment)

@incanus
Copy link
Contributor

incanus commented Mar 18, 2014

Sure. To be clear, I ask for a pull request of code for the feature so that 1) we can comment on it inline and work through possible acceptance and 2) so that folks who want the features are motivated to add them.

GitHub's reference:

@mohpor
Copy link
Author

mohpor commented Mar 18, 2014

Thanks, will take a look asap—
Sent from Mailbox for iPad

On Tue, Mar 18, 2014 at 8:48 PM, Justin R. Miller
notifications@github.com wrote:

Sure. To be clear, I ask for a pull request of code for the feature so that 1) we can comment on it inline and work through possible acceptance and 2) so that folks who want the features are motivated to add them.
GitHub's reference:

* https://help.github.com/articles/using-pull-requests

Reply to this email directly or view it on GitHub:
#422 (comment)

@mohpor
Copy link
Author

mohpor commented Mar 18, 2014

OK, I read those articles, now I'm ready for your pull request. But I'm not currently at work (GMT+3:30 time zone), I will address this tomorrow morning.Thanks

Sent from Mailbox for iPad

On Tue, Mar 18, 2014 at 8:50 PM, Mohammad Porooshani porooshani@gmail.com
wrote:

Thanks, will take a look asap—
Sent from Mailbox for iPad
On Tue, Mar 18, 2014 at 8:48 PM, Justin R. Miller
notifications@github.com wrote:

Sure. To be clear, I ask for a pull request of code for the feature so that 1) we can comment on it inline and work through possible acceptance and 2) so that folks who want the features are motivated to add them.
GitHub's reference:

* https://help.github.com/articles/using-pull-requests

Reply to this email directly or view it on GitHub:
#422 (comment)

@incanus
Copy link
Contributor

incanus commented Mar 18, 2014

You actually send the pull to us, via forking the project, making some commits to your copy, and then initiating a pull request to our copy. We then modify and/or accept it into our copy.

@mohpor
Copy link
Author

mohpor commented Mar 18, 2014

I'm trying to do what you asked for and I noticed that just like earlier today at work, when I clone map box git and checkout develop branch, SMcalloutView submodule is not right. I mean the Xcode project is pointing to the wrong SMCalloutView files (the old one and SMCalloutClassicView is not present). Please investigate that too.

@incanus
Copy link
Contributor

incanus commented Mar 18, 2014

On latest develop, things, should be ok. Make sure you git submodule --update to sync things up.

@mohpor
Copy link
Author

mohpor commented Mar 18, 2014

I've requested a pull and added my commits, hope I did it right! (My First pull request: very exciting)

@mohpor
Copy link
Author

mohpor commented Mar 19, 2014

git submodule update failed for SMCalloutView and git submodule foreach git pull origin master didn't do anything for the submodule in question. I still see the problem in updating SMCalloutView in develop branch. Please investigate it through cloning mapbox-ios-sdk somewhere new.
Thanks

@incanus
Copy link
Contributor

incanus commented Mar 19, 2014

Make sure you are doing a recursive clone / submodule update. See this gist for a successful run I just did: https://gist.github.com/incanus/3f63059e54e9ac2d0df8

@incanus
Copy link
Contributor

incanus commented Aug 27, 2014

Any movement here @mohpor?

@mohpor
Copy link
Author

mohpor commented Aug 27, 2014

I will look into it today and let you know.
I have this in my project but I will make a clean commit to merge in develop branch.

@mohpor
Copy link
Author

mohpor commented Aug 27, 2014

@incanus, done.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants