gabesmed / ember-leaflet

Ember + Leaflet = Fun with maps
gabesmed.github.io/ember-leaflet
MIT License
164 stars 35 forks source link

globals/ember-cli #68

Closed samselikoff closed 9 years ago

samselikoff commented 10 years ago

Hi @gabesmed - thanks for your work on this!

Any plans on making this es6 modules for easier use within ember-cli? Any way I can help with that?

miguelcobain commented 10 years ago

I could tackle this easily as I have been coding an ember-cli addon recently. Ember-cli is becoming more and more the "de facto" standard for ember apps. It has many many advantages.

However this would imply a complete change in the repo. People WILL want an Ember-cli addon sooner or later. Also, I don't think it would be good to have separate repositories for ember-leaflet. My suggestion is set up a new branch and work things there.

What's your opinion on this, @gabesmed?

drwlrsn commented 10 years ago

Just for some more context. When I started my Ember-Leaflet project, I used ember-cli to get things going. It wasn't overly painful, but there was some digging.

miguelcobain commented 9 years ago

@gabesmed Ember 2.0 is fully embracing ember-cli and ES6. We should get this converted into an Ember-cli addon. I've made a few, I can do that, but I need a few directions. It is also possible to have the build process to output a standalone version for those not using ember-cli yet.

Maybe a new branch on this repo? Another repo?

nickiaconis commented 9 years ago

@miguelcobain I'm behind you on converting to an ember-cli add-on. I think we should keep everything in one branch of this repo if possible. We should also continue to offer a stand-alone build for those not using ember-cli. Can we keep the same build process we have now while also converting to an add-on?

miguelcobain commented 9 years ago

I'm using a cool build process in https://github.com/miguelcobain/ember-cli-selectize

It is both an ember-cli addon and a standalone release. Heavily based on the awesome work by @pixelhandler http://pixelhandler.com/posts/develop-embercomponents-for-sharing-as-ember-cli-addons-a-practical-example

nickiaconis commented 9 years ago

That pixelhandler article is a good read. It would be great if we could keep some of the release automation features from ef4/liquid-fire/packaging that pixelhandler/ember-off-canvas-components removed, but otherwise their's looks like a good base.

miguelcobain commented 9 years ago

I suggest that, for a start, we should just "bite the bullet" and create a new ember cli addon in a separate repo/branch. Then we could work on packaging as a standalone library.

pixelhandler commented 9 years ago

@miguelcobain does a repo that only has packaging and release scripts need to be a ember-cli addon? How about just a plain npm package? Seems that the "standalone packaging" scripts are a broccoli build pipeline based on es6 modules, not necessarily and addon for addons.

miguelcobain commented 9 years ago

Ok, so what are we discussing here? Got a bit lost. Ember-cli addon with "standalone packaging" scripts vs ... ?

What I suggested was simply to port this project to an ember-cli addon. Then we add "standalone packaging" to it (Just like on liquid-fire or ember-off-canvas-components). What do you suggest?

pixelhandler commented 9 years ago

oh, I read "Then we could work on packaging as a standalone library" as make a npm package to add 'standalone packaging' in it's own repo, then any ember-cli addon could install the standalone-packaging npm package to use it. I briefly mentioned this to @ef4 (liquid-fire author) and it seemed like a good idea. no body has done that yet.

miguelcobain commented 9 years ago

That is an excellent idea, indeed!

nickiaconis commented 9 years ago

+1 for the packaging library. Also, @miguelcobain, I'd be okay with starting the add-on conversion in a separate branch. We can always merge it into master once it's stable.

gabesmed commented 9 years ago

Hey all thanks for your thoughts on this! I'd love to get this ember-cli compatible, but don't have a huge amount of time to dedicate to it. Willing to help out with whatever you think is the best approach!!

miguelcobain commented 9 years ago

@gabesmed, please create an orphan ember-cli-es6 branch, so I can PR my branch there: https://github.com/miguelcobain/ember-leaflet/tree/ember-cli-es6

Just to let people know of my progress and potentially to get some help.

miguelcobain commented 9 years ago

Also, just to keep this noted. I found this addon: https://github.com/csantero/ember-cli-ember-leaflet

It is basically a wrapper around the current ember-leaflet. However, a complete rewrite conforming ember-cli and ES6 modules, tests, etc would be much better and future proof, I think.

nickiaconis commented 9 years ago

The branch has been created.

I agree that a rewrite would be better than a wrapper. It's rather unfortunate they've already taken "ember-cli-ember-leaflet" on NPM, but I guess we can use "ember-leaflet" tagged with "ember-cli" or something like that.

Pulling in some info from #87: moving to components is a good idea, and I like that you were already taking this into consideration. Another option would be to provide both Mixins and Components for things (i.e. the MarkerLayer, which is a Component(?), mixes in the MarkerLayerMixin). This way users who really want to use Views can do so, but we don't have to support it. However, I don't want to drive this decision because the Mixin idea is a pet project of mine from a while ago. @gabesmed, what say you?

Also, here's another: https://github.com/csantero/ember-cli-leaflet It's already deprecated in favor of the wrapper around our library. I'm not sure how I feel about this. Somewhere between laughter and consternation.

miguelcobain commented 9 years ago

PR at #88

ember-cli-ember-leaflet it's not a good name anyway. :stuck_out_tongue_closed_eyes:

I think the only component we can have is the main ember-leaflet that renders the map. We never use Ember.View or Component anywhere else in EmberLeaflet. All this layer "architecture" in EmberLeaflet does not depend on Ember.View at all.

My idea was to let this ember-cli-es6 branch make its way to master, someday. I think it's doable without breaking anything (people already using this from bower, etc), especially once we have the "standalone packaging" thing worked out.

gabesmed commented 9 years ago

sooooo cool :D hooray for ES6!!

gabesmed commented 9 years ago

i'm fine with redoing the whole thing in ES6 BTW! awesome work getting this started!

@codefox421 I'm still not sold on having everything be a mixin, but I'm also a huge believer in the people doing the work having the ultimate decision. if you guys are leading a port to ES6 (which I wholeheartedly support but sadly do not have time to lead) I trust you to make the call.

the main reason I had for not making everything a mixin is not seeing the need for applying the functionality to any other class -- in the same way that an Ember.View (or Ember.Component as I understand it) is not a mixin, it is a base object. A key underlying principle of EmberLeaflet is to treat leaflet layers in the same way as Ember treats Views, as they are the same kind of object. I think it's a really useful analogy in decisions like these.

miguelcobain commented 9 years ago

A key underlying principle of EmberLeaflet is to treat leaflet layers in the same way as Ember treats Views

I think it was the genius principle of the project. :+1:

lucacorti commented 9 years ago

Hello,

What is the status of the ember-cli/es6 port wrt master?

This would simplify working and hacking on EmberLeaflet a lot. Any plans to switch the official release to this branch?

thanks

gabesmed commented 9 years ago

@lucacorti @miguelcobain is finishing up the https://github.com/gabesmed/ember-leaflet/tree/ember-cli-es6 branch....i'll check in on it!

lucacorti commented 9 years ago

Great, thanks a lot, awesome work guys.

lucacorti commented 9 years ago

I'm trying this out, using the default {{ember-leaflet}} component in the templates works just fine, but if I try and extend the EmberLeaflet component like this:

import EmberLeafletComponent from 'ember-leaflet/components/ember-leaflet';
import MarkerCollectionLayer from 'ember-leaflet/layers/marker-collection';
import MarkerLayer from 'ember-leaflet/layers/marker-collection';
import PopupMixin from 'ember-leaflet/mixins/popup';
import CollectionBoundsMixin from 'ember-leaflet/mixins/collection-bounds';
import TileLayer from 'ember-leaflet/layers/tile';

export default EmberLeafletComponent.extend({
...

At runtime I get:

Uncaught Error: Could not find moduleember-leaflet/components/ember-leafletimported frommy-app/components/ember-leafletloader.js:110

Is this expected?

miguelcobain commented 9 years ago

It should definitely work. How did you install the addon in your app?

lucacorti commented 9 years ago

Added this to package.json

"ember-leaflet": "git://github.com/lucacorti/ember-leaflet.git#ember-cli-es6"

And then run:

npm install
ember g ember-leaflet
miguelcobain commented 9 years ago

Ember-cli version?

That should work.

lucacorti commented 9 years ago

0.2.0, should I perhaps try ember install:addon git://github.com/lucacorti/ember-leaflet.git#ember-cli-es6 ?

miguelcobain commented 9 years ago

That should be equivalent.

Although exciting, this branch is a WIP and untested, so bear with us, please. But congrats for taking the deep dive here. :+1:

lucacorti commented 9 years ago

Sure, I'll let you know if I find out more about this issue.

lucacorti commented 9 years ago

I can reproduce this on a new app using ember-cli 0.2.0:

ember new my-app && cd my-app
ember install:addon git://github.com/lucacorti/ember-leaflet.git#ember-cli-es6
ember g ember-leaflet
ember g component my-map

And add to templates {{ember-leaflet}} (works), {{my-map}} (extending EmberLeafletComponents), fails with above error.

lilfaf commented 9 years ago

+1 hi guys, I encounter the problem as @lucacorti. I tried install on ember-cli 0.2.1and 0.2.3 and never managed to get it working when extending EmberLeafletComponents.

Awem commented 9 years ago

I created a PR to fix the issue with extending the component: https://github.com/gabesmed/ember-leaflet/pull/107

miguelcobain commented 9 years ago

@lucacorti and @lilfaf, it should be possible now.

miguelcobain commented 9 years ago

@pixelhandler and @codefox421 looks like that addon we mentioned in this issue (https://github.com/gabesmed/ember-leaflet/issues/68#issuecomment-70298803) has been indeed created by @ef4!

Ember-giftwrap: https://github.com/ef4/ember-giftwrap

Anyone is welcome to integrate it with ember-leaflet. :) Created #108.

saerdnaer commented 9 years ago

When I try to install to use this branch with ember-cli 0.24 I get following error message:

ember new my-app && cd my-app
ember install gabesmed/ember-leaflet#ember-cli-es6
 version: 0.2.4
 Installed packages for tooling via npm.
 Cannot read property 'name' of undefined
 TypeError: Cannot read property 'name' of undefined
miguelcobain commented 9 years ago

@saerdnaer please try this method meanwhile: https://github.com/gabesmed/ember-leaflet/pull/111#issuecomment-102327290

saerdnaer commented 9 years ago

@miguelcobain This worked after adding *.tiles.mapbox.com in ENV.contentSecurityPolicy.img-src in config/environment.js, adding {{leaflet-map}} to app/templates/application.hbs and setting an height > 0 for this element with the chrome inspector manually. Im still searching for an way to persist this height setting.

Awem commented 9 years ago

@saerdnaer just add this to your app's css if you want a height of e.g. 400px:

.map-container {
  height: 400px;
}
saerdnaer commented 9 years ago

@AW-UC Thanks, that worked. I had only to replace map-container with leaflet-container.

In case anyone is interested: Here are all my changes to getting the thing running: https://github.com/saerdnaer/ember-cli_leaflet_example/commit/d7fb7adf2bd4185757c2412b50091d85ad86364d

miguelcobain commented 9 years ago

We'll need an entry about this in the README. People get bumped into that a lot.

Awem commented 9 years ago

@saerdnaer yes, I'm sorry. Using map-container of course requires, that you wrap the component in

<div class="map-container">
</div>

IMHO, you should do that, so that you don't set the width of leaflet-container globally to a specific value. For example, you can set:

.map-container .map1 {
  height: 400px;
}
.map-container .map2 {
  height: 500px;
}
.leaflet-container {
  width: 100%;
  height: 100%;
}

Then you wrap your different maps accordingly. This is very reusable and also works well in responsive layouts.

saerdnaer commented 9 years ago

If someone wants to try out having a leaflet google maps layer, have a look at this commit: https://github.com/saerdnaer/ember-cli_leaflet_example/commit/b8452047753ccb574a88014baddc11a8fb076375