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

Refactor toolbars, add "delete last point" action (fix #229, #236 and probably some others) #242

Merged
merged 6 commits into from
Dec 19, 2013

Conversation

Zverik
Copy link
Contributor

@Zverik Zverik commented Dec 10, 2013

This also fixes css for long action titles, so action buttons do not wrap.

removeToolbar: function () {
L.Toolbar.prototype.removeToolbar.call(this);

onToolbarRemove: function () {
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so it doesn't override parent method, but acts as a listener called from it, consistent with onToolbarAdd.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I was more meaning why do it this way rather than override the parent method and call L.Toolbar.prototype.addToolbar. If it is just a style choice that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, overriding a methods means you have to understand how parent method works and not forget to call it properly. Listener method is an API, which means it have clearly defined input and output that can be documented somewhere. The less stuff need to be overridden — the better.

@jacobtoye
Copy link
Member

This is an amazing pull! Thanks so much @Zverik

@Zverik
Copy link
Contributor Author

Zverik commented Dec 17, 2013

I've deployed the patched Leaflet.draw to http://mapbbcode.org — press "Edit" button to see how the new button looks.

jacobtoye added a commit that referenced this pull request Dec 19, 2013
Refactor toolbars, add "delete last point" action (fix #229, #236 and probably some others)
@jacobtoye jacobtoye merged commit 298eee8 into Leaflet:master Dec 19, 2013
@Zverik Zverik deleted the undo branch December 19, 2013 06:32
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.

2 participants