search

eschwartz / backbone.googlemaps

A Backbone JS extension for interacting with the Google Maps API (v3.10)
MIT License
139 stars 55 forks source link

overlayOptions not copied to MapView.overlayOptions #24

Closed twhtanghk closed 10 years ago

twhtanghk commented 10 years ago

MapView.overlayOptions is initialized as empty map. The condition "this.overlayOptions || (this.overlayOptions = options.overlayOptions)" for checking always return true if it is an empty map. It is suggested to revised as below. Any other suggestion.

_.extend(this.overlayOptions, options.overlayOptions);
twhtanghk commented 10 years ago

At last, I remove the static variable overlayOptions: {}; and replace the following assignment of member variable in constructor. 

this.overlayOptions = options.overlayOptions || {};
eschwartz commented 10 years ago

That should do it for you.

I prefer:

_.extend(this.overlayOptions, options.overlayOptions);

as this allows you to set the instance variable as defaults, then override those defaults with options in the View constructor.

twhtanghk commented 10 years ago

I try _.extend(this.overlayOptions, options.overlayOptions); before, but I found that the static variable would apply to all instances. That is, the last updated overlayOptions would apply to next created instance. Any other suggestion. Thanks.

eschwartz commented 10 years ago

You're right -- Backbone.extend extends the child classes prototype, which means that any changes to an instance property defined in extend will be applied to all instances of the class. Best practice here would be to assign all instance properties within the initialize method, instead

This is something that should be fixed for all the classes in the library. I can't get to it today, but it's going to the top of my list (unless you want to submit a pull request :) ).

eschwartz commented 10 years ago

Sorry for not updating this, but I believe this issue is resolved now.

See e8a52a5a57b6d5afc4837e270466cde7e73ddd93