Skip to content

Conversation

jfirebaugh
Copy link
Member

This throws an error:

var map = L.map('map');
L.control.scale().addTo(map);​

http://jsfiddle.net/CtJu7/1/

Can we special-case map.on('load', callback) to work like jQuery load, triggering the callback immediately if the map has already been loaded? Then Scale#onAdd could do:

map.on(options.updateWhenIdle ? 'moveend' : 'move', this._update, this);
map.on('load', this_update, this);

And this code could be refactored.

I'll send a PR if you agree.

@mourner
Copy link
Member

mourner commented Oct 12, 2012

Sounds useful, although I don't like having two different behaviors in one interface depending on some hardcoded event types. Need to think this through.

@ghost ghost assigned mourner Oct 12, 2012
@mourner
Copy link
Member

mourner commented Oct 12, 2012

At the moment I think I'll just add map._loaded check in the Scale control and will refactor in future.

@jfirebaugh
Copy link
Member Author

I turned this into a PR that shows how this can be done pretty cleanly. See what you think.

} else {
events = this[key] = this[key] || {};
events[types] = events[types] || [];
events[types].push({
Copy link
Member

Choose a reason for hiding this comment

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

Did you change this to avoid calling splitWords every time?

@jfirebaugh
Copy link
Member Author

See this commit message for the rationale for those changes.

@mourner
Copy link
Member

mourner commented Oct 12, 2012

I see now. Still don't quite like the addEventListener overloading approach — it's quite confusing, as you normally wouldn't expect the listener to be immediately called when attached — this detail is hidden in implementation. E.g. if you call $(document).bind('ready', ...), the listener won't be called if DOMReady already happened, unlike explicit ready method.

How about adding a special method for map instead of overloading addEventListener? I mean, something like:

map.whenReady(this._update);

It would be simpler both in API and implementation.

@jfirebaugh
Copy link
Member Author

That's a good point -- I'll implement what you suggested.

@mourner
Copy link
Member

mourner commented Oct 12, 2012

Please revert the alias changes then. I'm a perfectionist, what can be written shorter and simpler should be done so. :)

@jfirebaugh
Copy link
Member Author

Done.

mourner added a commit that referenced this pull request Oct 12, 2012
can't add scale control to uninitialized map
@mourner mourner merged commit d2af375 into Leaflet:master Oct 12, 2012
@mourner
Copy link
Member

mourner commented Oct 12, 2012

Looks good!

emartinez-usgs pushed a commit to emartinez-usgs/Leaflet that referenced this pull request Feb 27, 2013
emartinez-usgs pushed a commit to emartinez-usgs/Leaflet that referenced this pull request Aug 15, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants