The Great Tests Crusade

[miguelcobain]

Added tests for:

• collection
• container
• marker
• tile
• collection bounds mixin
• draggable mixin

They're green! Loads of fun!

Don't merge yet. I'll do the rest tomorrow.

[gabesmed]

:D 👍

[miguelcobain]

@twokul @gabesmed Any idea on how we can test template and view lookups?
I'm having some trouble porting this complex view test.

You specified the templateName customPopup for <(subclass of Ember.View):ember1181>, 
but it did not exist.

Looks like the App/container created by moduleForComponent isn't looking at Ember.TEMPLATES?

[miguelcobain]

Only the following tests are missing:

• popup_complex_view_test.js
• marker_cluster_tests.js
• register locationsEqual helper properly

We currently don't have MarkerClusterLayer as well. It isn't part of the leaflet library, but it is an official plugin.
Do you think we should include this plugin in EmberLeaflet?
Also, I need some help in popup_complex_view_test.js like I said before.

[miguelcobain]

This is a delicate part.
Simple tests are passing, but I would like to know if there is a better way to do this.

[twokul]

@miguelcobain we should probably move the code from app under addon directory. I did a lot of tests porting yesterday but it seems like you can finish it up and I will just rebase.

[twokul]

@miguelcobain @gabesmed we need to decide if we want to expose things like TileLayer as components

[miguelcobain]

Right now we are just exposing them as layers (normal Ember classes).
Anyone can import TileLayer from 'ember-leaflet/layers/tile'; in their apps, which feels right to me.

What would be the advantage of exposing them as components?

[twokul]

{{#ember-leaflet}}
  {{ember-leaflet-tile}}
  {{ember-leaflet-tile}}
  {{ember-leaflet-tile}}
{{/ember-leaflet}}

@miguelcobain

[miguelcobain]

OUCH.

That would render dom structure, for sure.
Then we can scan that DOM and reflect it in a map (that is, a complete ember-leaflet class hierarchy)?

Dealing with changes and {{each}} may not be an easy task, but I suddenly became thrilled with the amazing possibilities.

Suggestion: keep the current class system and create additional components that wrap each class:

• current tests still validate our main logic
• we just need to code the sync between DOM <-> ember-leaflet classes

This is assuming we take the DOM approach.
This is like the embodiment of the @gabesmed's idea of using the ember views structure in leaflet layers.

[gabesmed]

really neat idea @twokul, definitely see that components are the way ember is moving. however it might get very hard to create complex customizations inside HTML. Would this be ember idiomatic? How would you customize properties and create computed calculations?

{{#ember-leaflet}}
  {{ember-leaflet-tile url="...." optionA="something"}}
  {{ember-leaflet-markers content=controller.markers locationProp="content.loc"}}
  {{ember-leaflet-polygon radius=50 center=controller.center}}
{{/ember-leaflet}}

also @miguelcobain has a good point that this would end up creating DOM, which we'd then have to parse and eliminate. ewww.

maybe we could move all configuration away from declaring classes into like a json object.

{{ember-leaflet layers=controllers.layers}}

Controller = Ember.Controller.extend({
  layers: [
    EmberLeaflet.TileLayer.extend({url: ...});
    EmberLeaflet.MarkerCollectionLayer.extend({....})
  ]
});

In any case, we should definitely consider interfaces changes in a separate thread from the es6 module port. Let's keep this a straight port without changing functionality just to pull off the bandaid ASAP. After this is done it'll be much easier to do branches and consider alternate interfaces

[miguelcobain]

@gabesmed

{{#ember-leaflet}}

  {{tile-layer url="...." optionA="something"}}

  {{#collection-layer}}
    {{#each marker in controller.markers}}
      {{marker-layer locationProp=marker.loc}}
    {{/each}}
  {{/collection-layer}}

  {{polygon-layer radius=50 center=controller.center}}

{{/ember-leaflet}}

Maybe a bit more "idiomatic".
Maybe "scanning the DOM" wouldn't be necessary. I don't know if Ember keeps any accessible object hierarchy internally that reflect the template.

Agree with getting this port done and taking this to the next stage.

[twokul]

@gabesmed 👍

[miguelcobain]

Guys, check my branch: https://github.com/miguelcobain/ember-leaflet/tree/component-render

I've managed to render a map like this:

{{#ember-leaflet}}
  {{#if tileEnabled}}
    {{tile-layer tileUrl="//a.tiles.mapbox.com/v3/examples.map-zr0njcqy/{z}/{x}/{y}.png"}}
  {{/if}}

  {{#container-layer}}
    {{#each location in enabledLocations}}
      {{marker-layer location=location.location}}
    {{/each}}
  {{/container-layer}}
{{/ember-leaflet}}

ifs and eaches are working. I can toggle ifs and add and remove to arrays. I can nest layers as deep as I want. Example:

{{#ember-leaflet}}
  {{#if tileEnabled}}
    {{tile-layer tileUrl="//a.tiles.mapbox.com/v3/examples.map-zr0njcqy/{z}/{x}/{y}.png"}}
  {{/if}}

  {{#container-layer}}
    {{#container-layer}}
      {{#container-layer}}
        {{#each location in enabledLocations}}
          {{marker-layer location=location.location}}
        {{/each}}
      {{/container-layer}}
    {{/container-layer}}
  {{/container-layer}}
{{/ember-leaflet}}

It is just a quick proof of concept, but it's definitely doable!!!
Didn't have to scan any DOM. I just use the component/view lifecycle itself to create layers. I'm really thrilled with the possibilities. I really like to create maps like I create HTML. 😄

[nickiaconis]

@miguelcobain Love that template! 👍 Though as @gabesmed suggests, it is worth its own PR.

[twokul]

@miguelcobain looks good dude

[miguelcobain]

@codefox421 oh, absolutely.
I just couldn't resist to have quick take on this. 😆

[miguelcobain]

#98 (comment):

We currently don't have MarkerClusterLayer as well. It isn't part of the leaflet library, but it is an official plugin.
Do you think we should include this plugin in EmberLeaflet?

I think I'm dropping support for marker clusters. This is a leaflet plugin. It could be packaged as an ember-leaflet addon later. Do we agree?

[miguelcobain]

After reading this: emberjs/ember.js#5534
I don't think we can provide the popupView feature. ifs don't work, actions, etc, probably don't work as well. The test is failing (only one test) because the popup template doesn't update. You're free to try to make it pass.
Perhaps we need to think of another way to provide this functionality.

[gabesmed]

Is there any way to include it without EmberLeaflet as a whole relying on the markercluster plugin? as in it throws an error if you try to use the markercluster functionality without including the plugin, but the entire library won't crash?

think it's a pretty commonly used plugin, and I selfishly need it for my use case :)

[miguelcobain]

It is possible to detect its presence and throw an error if someone tries to use our class without importing the plugin. I never used this plugin, though maybe I should have. :)

[gabesmed]

I'd prefer that if it doesn't cause too much trouble. But open to overrulling -- not a big deal either way. :) I see the argument for clean-ness, but also think ember-leaflet is maybe too small a library to start having plugins of its own.

regarding the popupView, don't we just use ember's DOM functionality? don't see how this wouldn't still work -- we're just basically copying the view's DOM element?
https://github.com/gabesmed/ember-leaflet/blob/master/packages/ember-leaflet/lib/mixin/popup.js#L73

[miguelcobain]

The popup appears correctly.
The problem is that bindings and many template helpers don't work, so it doesn't update to reflect the changes. That's the only test that's failing.

Also, a lot has changed. That code you mentioned currently throws:

 'undefined' is not a function (evaluating 'this._popupView._insertElementLater(function () {
                    self._popupView.$().appendTo(self._popup._contentNode);
                    self._popup.update();
                  })')

Looks like_insertElementLater no longer exists, maybe.
I've managed to do this quite elegantly, after reading @mixonic comment emberjs/ember.js#5534 (comment) like:

this._popupView.createElement();
this._popup.setContent(this._popupView.element);
this._popup.update();

But this doesn't pass the tests. The template doesn't update. :/
Maybe that's expected, I don't know.

Need some help here to either make this work or decide an alternative solution if we conclude this doesn't work anymore.

[gabesmed]

@miguelcobain ok i'm looking into popup view test...maybe i can help

[gabesmed]

@miguelcobain hm, looks like it's partially working.

see how the first test is failing, but the second test is succeeding, and how the updated marker shows the #20? (i manually added a stop() statement in the test so that I could debug in the live browser)

looks like the for loop in the template is updating just fine. it's the if statement that isn't working

[gabesmed]

good news is this means the basic approach works? bad news is now we need to debug the if statement specifically. alllllmost there!!

[gabesmed]

TESTS PASSING :D -- is this all the tests ported over now @miguelcobain cc @twokul

[ssured]

Hey Guys, have you seen the approach in https://github.com/jerel/poc, more specific jerel/poc#1 (comment)? For me his code works really well. Rendering content in a popup is rather simple using that code

[nickiaconis]

Another thing to consider is that with HTMLBars you can let Ember render an element whereever it likes on the page, then move it somewhere else, and Ember will continue to update it as if it never was moved. This wasn't possible before because of those metamorph script tags Handlebars injected everywhere.

[ssured]

@codefox421 : yeah that is the exact approach which is used by @jerel.
I think exploring these new possibilities is really worth the effort. I have some code which I am willing to open-source. It will be a non backwards compatible change for the ember-leaflet repo though. @gabesmed what is your opinion on this, as the owner of the current repo & project?

[miguelcobain]

@ssured The current implementation only uses one private API call as well.
I'd love to know the advantages/disadvantages of both approaches.

[ssured]

@miguelcobain I put up my code on github. It is not very polished but does the job. See https://github.com/ssured/ember-cli-leaflet.

https://github.com/ssured/ember-cli-leaflet/blob/master/addon/components/leaf-base.js exposes an attach and detach method, which get implemented for several UI elements supplied by Leaflet, see for example https://github.com/ssured/ember-cli-leaflet/blob/master/addon/components/leaf-ilayer.js#L24-L49. This is the same pattern pointed out by jerel/poc#1 (comment).

There is a demo app living in the gh-pages branch. http://ssured.github.io/ember-cli-leaflet/
What you can do here is pan around on a mapbox map. Clicking a coordinate navigates to a new route with the geohash as a parameter. The bounding box of the geohash is rendered (you can see a rect at zoom 18) and a marker is rendered. When you click the marker, a popup opens, with a bound counter property updated every second. See https://github.com/ssured/ember-cli-leaflet/blob/master/tests/dummy/app/templates/application.hbs and https://github.com/ssured/ember-cli-leaflet/blob/master/tests/dummy/app/templates/position.hbs for how the different leaflet controls can be nested into each other.

Does this align with your project?

[miguelcobain]

Definitely.

I think adding a component rendering logic to the existing codebase shouldn't be too hard.
Also, I think this is aligned with what we want for the next version, once the ember-cli-port lands on master.