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

Examples are broken on master #8

Closed
kyletolle opened this issue Jan 15, 2015 · 6 comments
Closed

Examples are broken on master #8

kyletolle opened this issue Jan 15, 2015 · 6 comments

Comments

@kyletolle
Copy link
Contributor

I've cloned the Leaflet.toolbar repo, am on the master branch, and done an npm install like the README instructs. However, when I view the examples (popup and control) in my browser (Safari 8.0.2 on Yosemite), neither of them works.

The popup example gives me this error: TypeError: undefined is not an object (evaluating 'L.Edit.Popup.Edit')

The control example gives me errors about loading CDN'd CSS and JS files, as well as this error: TypeError: undefined is not an object (evaluating 'new L.DrawToolbar.Control')

Now, if I check out the gh-pages branch, the examples do work. But to have the examples broken on the master branch ends up being confusing. And it's not evident that I need to check out gh-pages to get working examples.

Could the master branch examples be updated to work?

@kyletolle
Copy link
Contributor Author

Further, if I try to implement the popup example myself by following the steps on the Building custom toolbars wiki page while using the master branch's dist/Leaflet.Toolbar.js, things just don't seem to work.

You can see an example in my gist. And a demo at the bl.ocks.org demo site.

(Each of the css and js files was taken from that repo's respective master branch.)

I get the error TypeError: undefined is not a function (evaluating 'action._createIcon(this, this._ul, this._arguments)').

This seems to be evidence that Leaflet.toolbar's master branch is broken.


I've also tried doing npm install leaflet, npm install leaflet-toolbar, and npm install leaflet-draw and using the css and js from the node_modules in my index.html, but things are still broken. I continue to get the error: TypeError: undefined is not a function (evaluating 'action._createIcon(this, this._ul, this._arguments)').


All in all, I'm not able to get an example using Leaflet.toolbar and Leaflet.draw working at all. Something's special about that gh-pages branch, but I'm not really sure what. I hope my information helps lead to a fix. I'd love to be able to use leaflet, toolbar, and draw for using the popups in my application, once it's back to working!

@justinmanley
Copy link
Member

Hey Kyle - sorry about this. I understand that it's not intuitive behavior.

The examples for Leaflet.toolbar all draw on Leaflet.draw, which is a separate Leaflet plugin. Leaflet.draw currently uses its own toolbars, not built using Leaflet.toolbar. For this reason, the Leaflet.draw dependency in the Leaflet.toolbar package.json file actually points to a feature branch of my own fork of Leaflet.draw: manleyjster/Leaflet.draw.

The examples page loads the dist files for Leaflet.draw (that is, dist/leaflet.draw.js and dist/leaflet.draw.css in the Leaflet.draw repo). @jacobtoye has asked me not to include those dist files in previous PR's, so, unfortunately, when you npm install Leaflet.toolbar, those files don't reflect my additions to Leaflet.draw.

This should be resolved once the PR is accepted and we push a new release of Leaflet.draw.

In the meantime, the easiest solution for you is just to build Leaflet.draw from source after you install Leaflet.toolbar. In the root of Leaflet.toolbar, run: cd node_modules/leaflet-draw && jake build.

I may come back to this over the next few days and modify the files that are loaded in the example so that Leaflet.draw/src is loaded, rather than the (out-of-date) files from Leaflet.draw/dist.

Again, sorry about this.

@kyletolle
Copy link
Contributor Author

Thanks for the quick response @manleyjster! No need to apologize. I had no idea I was attempting to use Leaflet.toolbar and Leaflet.draw together at a time of transition. Very exciting to see this change that's coming!

I knew that Leaflet.draw was a different library, but I naively assumed that Leaflet.toolbar just overrode or plugged into Leaflet.draw. I'm quite new to Leaflet and its plugins, so it's been a learning experience.

I appreciate the extra information. I now see the package.json pointing to your branch of the Leaflet.draw plugin. And it also makes sense as to why things weren't working for me. On the gh-pages branch, it seems you've committed some files under node_modules, which I'm guessing include your modified Leaflet.draw.

Great, I'll see what I can do with building from source. I might also focus on some other pieces of my application for now, until these Leaflet.draw and Leaflet.toolbar modifications are officially released. I've also taken some time to read up on the PR, and your blog post for the work you've done.

I'm certainly excited to use this code, so I look forward to its release!

@kyletolle
Copy link
Contributor Author

Your instructions to build the leaflet-draw source worked to get the popup example working locally on the master branch. Thanks!

@cyrilchapon
Copy link

cyrilchapon commented Sep 21, 2016

The version of the file used in example (here) doesn't exist at all in your fork repository neither in leaflet-toolbar or leaflet-toolbar-plugin or leaflet-toolbar-plugin-patches or even external-leaflet-toolbar.

None of the latest versions of those branches actually work.

@justinmanley
Copy link
Member

Marking as fixed because the workaround described in this bug is no longer required with the release of https://github.com/justinmanley/leaflet-draw-toolbar.

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

No branches or pull requests

3 participants