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

PlacesService:radarSearch() added #179

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

PlacesService:radarSearch() added #179

wants to merge 6 commits into from

Conversation

cancerberoSgx
Copy link

added missing method radarSearch() in PlacesService

* @param handler
*/
public final native void radarSearch(PlaceSearchRequest request, PlaceSearchHandler handler) /*-{
var callback = function(results, status, pagination) {
Copy link
Owner

Choose a reason for hiding this comment

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

formatting

@branflake2267
Copy link
Owner

Good job!

Could you add a test case for he radar search?

Is there other benefits to interfacing the layers?

@cancerberoSgx
Copy link
Author

Please include only the commit about radarSearch() (commit b4d264b ). I will try to include use cases in your test for that.

The other commit about Layer super type s wrong and it won't compile in GWT (an interface can be implemented only by ONE overlay type). I?m trying to revert it in my cloned repo - i'm just starting with git/github. IMHO it was a good idea because you can reference all the layers using the same supertype (for example putting several layer objects in the same collection and access them using the same interface). But unfortunately it seems we cannot "do polymorphism" with JSOs.More info https://groups.google.com/d/msg/google-web-toolkit/iu3GHi4nWK4/bVE-agIAe-kJ

@branflake2267
Copy link
Owner

Could create a new branch and cherry pick that commit to it and ask for another pull? My first thought with the interface is it's a little overkill and I don't see the need for it other than code boil down. Thats why I asked if there was some other benefit other than making it easier to write the library. :) I like the idea. This api is already has a heavy inheritance structure with the MVC class wrapper. I tried to keep the wrapping to minimum and pass along some of it. :)

@cancerberoSgx
Copy link
Author

Hi Brandon, while I'm looking information about how to "create a new branch and cherry pick that commit" (because I'm just starting with git), I added another commit to this pull request that

  1. revert all the changes related to Layer super type
  2. add a newInstance() constructor to PanoramioLayer that takes no arguments. Yes I know, you can just pass null as the argument, but all the other Layer types support newInstace() without the options (optional) argument.

@branflake2267
Copy link
Owner

What I might do is create a new branch using egit with no merge off the master, be sure to fetch all changes first. Then copy and paste in the changes to it and pull a new request. I'm not a pro at git, so I haven't done a cherry pick yet, but I see others talking about it like its easy all the time. :) Or just delete the layer interface from your code and commit.

#2 sounds like a good idea overloading the constructor.

@branflake2267
Copy link
Owner

This looks good so far.

Could you format the radar search?
Are you able to provide a test case for radar search?

@cancerberoSgx
Copy link
Author

I'm on it, thanks for the patience ;)

@branflake2267
Copy link
Owner

I don't mind at all... Great job! :)

@treethinker
Copy link

I've finally taken on conversion from maps 1.1 to 3.X, and am pretty tickled
with how easy the transition has been and how much I have to gain.
Thanks!

I'm curious, is it possible to remove a control from the map once you have
done mapwidget.setControl(location, widget) ??
I was trying to emulate a popup I had before, but I like the setControl mechanism.

If that's just a question of implementation, I might clone the repo and try to help.
Thanks,
Jerry Fowler

On 9 Apr,13, at 12:30, Brandon Donnelson wrote:

I don't mind at all... Great job! :)


Reply to this email directly or view it on GitHub.

@branflake2267
Copy link
Owner

Thanks. Good question. It's been a while since I've done that but removing it should be like adding.

radarSearch() and textSearch() in
showcase's testing.maps.PlaceSearchMapWidget
@cancerberoSgx
Copy link
Author

as you can see I keep fixing some details I'm finding when developing my app based on this project. Hope this is not too untidy for you :)

@branflake2267
Copy link
Owner

Looks good to me :) good job!

@branflake2267
Copy link
Owner

Do you think you could catch this up so I could merge?

@cancerberoSgx
Copy link
Author

Short of time these days but I will do it in the next 2 weeks - regards!

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.

3 participants