issue 226 google ready fixes

[brunob]

Hi and thanks for the patch, few questions/remarks :

• think the .gitignore is not usefull
• do we really need the karma.conf ans all these new dev dependencies in the package.json ?

The patch seems a bit disproportionate for a "simple" fix like that, this specific change could do the job alone, no ?

https://github.com/shramov/leaflet-plugins/pull/231/commits/ce454c6736c9c1d44aa776ce3ec5987602f47061#diff-9a15ae9cb46f6cad34cf696d87b8fbd5

Anyway, if you confirm that the complete refactoring of google.js is needed, we should merge it :)

[nmccready]

The refactoring is necessary as it was not working on mobile at all. Now it works. As far as karma dependencies they are only devDependencies so this puts no strain on people using leaflet-plugins. IE it is not downloaded by them.

[brunob]

Ha, strange that nobody took care of signaling that google layer was not working at all on mobile...

By my side, i repeat, i don't want to spare time on google layer script, so thanks to propose yourself :)

For the dev dependencies, why should we merge these since you are the only one to use it ? Maybe you could put these in your fork or you local copy. It would allow to merge changes on the only concerned fil, i think this would be "cleaner".

[nmccready]

The purpose of the tests are to validate the code base. They are less for me and you but to prevent other contributors from breaking the code base. Therefore on changes they must fix the tests and or six the source. To me it would be unwise to not have them. This project could use a lot more specs.

Sent from my mobile device

VR,

Nicholas McCready twitter:nmccready github:nmccready

On Jul 22, 2016 10:42, "b_b" notifications@github.com wrote:

Ha, strange that nobody took care of signaling that google layer was not working at all on mobile...

By my side, i repeat, i don't want to spare time on google layer script, so thanks to propose yourself :)

For the dev dependencies, why should we merge these since you are the only one to use it ? Maybe you could put these in your fork or you local copy. It would allow to merge changes on the only concerned fil, i think this would be "cleaner".

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/shramov/leaflet-plugins/pull/231#issuecomment-234562764, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjnqXsaDyLuzQfEUTy5GlXQPYj694NGks5qYNbsgaJpZM4JSCUf .

[brunob]

They are less for me and you but to prevent other contributors from breaking the code base.

They will impact me too, since i would have to run these tests before merging anything. If i don't do that, i don't really see the point to provide them.

[brunob]

@nmccready nice compromise, feel free to update your for and push this change in order to allow me to merge this PR.

[nmccready]

@brunob done , rebased and squashed

[brunob]

@nmccready just merged when you posted your last comment :\ I can wait a bit before tagging an publish to npm if you want.

[nmccready]

@brunob I think the question (if there is any fix) is out of scope for this. I would cut a release and open a new issue to discuss what can be done.

[brunob]

Ok, going to tag and publish, thx for the patch :)

[seyfro]

@nmccready thanks a lot for the patch!

I already tried to implement it but found an issue as I also use the deferred loading example from https://github.com/shramov/leaflet-plugins/blob/master/layer/Layer.Deferred.js respectively https://github.com/shramov/leaflet-plugins/blob/master/examples/deferred.html

If you also use Layer.Deferred.js, Google Maps need to be initiated according to the example deferred.html in the following way: https://maps.googleapis.com/maps/api/js?callback=L.Google.asyncInitialize - anyway this pull request removes that callback and if I try to open a Google maps, I am getting the error InvalidValueError: L.Google.asyncInitialize is not a function - if I on the other hand remove ?callback=L.Google.asyncInitialize maps do load load at all.

Any hint on how to make the updated Google.js codebase compatible with Layer.Deferred.js again would be highly appreciated!

[nmccready]

@robertharm I'll take a look, it should work as I have been using this code for well over month with no issues.

[nmccready]

@robertharm the thing is you no longer need &callback=L.Google.asyncInitialize. You can assign it to whatever callback you like. So yes the example is broken.

However now all you need to do is attach to L.Google._googleApiPromise and when that promise is resolved google maps is good to go. this._googleApiPromise

[nmccready]

@brunob I guess a bump to 2.X might have been better as this was a breaking change.

[brunob]

Ha, not so cool... anyway, now it's done, can @nmccready please fix the broken example before i release a 2.x ?

[seyfro]

@nmccready removing &callback=L.Google.asyncInitialize from the deferred.html example is not enough unfortunately - map is not being loaded as a result...

[nmccready]

@brunob yeah my bad I didn't know there was an example for it.

[brunob]

@nmccready no problem, we just have to find a way to fix this example, no hurry :)

[nmccready]

@brunob https://github.com/shramov/leaflet-plugins/pull/242

@robertharm