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

project page + examples #111

Closed
wants to merge 3 commits into from
Closed

project page + examples #111

wants to merge 3 commits into from

Conversation

miguelcobain
Copy link
Collaborator

I've started the project page.

But I've encountered some problems converting the old examples to the new ember-cli.
I created this PR to get everyone's suggestions.

  • we need 3 features and descriptions
  • most importantly, we need to have some examples
    • example 1
    • example 2
    • example 3

Here is how the page looks right now: preview

@miguelcobain
Copy link
Collaborator Author

@AW-UC and people who are using this branch of ember-leaflet:

What is your general strategy for passing data to the {{leaflet-map}} component?

@Awem
Copy link
Contributor

Awem commented Apr 23, 2015

I first extend the component with options I need for that specific map view. Then I pass data into the component by binding a property like {{leaflet-map place=model variables=leafletService}} (you could e.g. also bind to properties of the route). Then I can acces those properties in the extended component like this: this.get('place').get('latitude') etc. Sometimes it can also be useful to not use models but have a service where you get and set values. You can inject it into the component and access it the same way you access models.

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

Here is a more detailed description on how to deal with it:
Let's say wh have a model place, with attributes like lat and lng (floating point). I now add a computed property to it like this:

//app/models/place.js    

/* global L */
import DS from 'ember-data';
export default DS.Model.extend({
  //...other attributes...
  location: function() {
    var x = this.get('lat');
    var y = this.get('lng');
    return L.latLng(x, y);
  }.property('lat', 'lng')
});

I want to render the map on places/{id}. On the corresponding js for the route I include something like:

export default Ember.Route.extend({
  model: function() {
    return this.store.find('place');
  }
});

Now in the template for this route I call the component:

{{leaflet-map place=model tileUrl=leafletService.tileUrl}}

If you don't want to do much with place in the component, then it would be even better to just set all input on the render call:

{{leaflet-map location=model.location tileUrl=leafletService.tileUrl}}

Then the get calls in the component would be even simpler.

A correspondent component would look like this:

/* global L */
import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';
import MarkerCollectionLayer from 'ember-leaflet/layers/marker-collection';
import TileLayer from 'ember-leaflet/layers/tile';

export default EmberLeafletComponent.extend({
  center: function(){
    return this.get('place').get('location');
  }.property('location'),
  childLayers: [
    TileLayer.extend({
      tileUrl: function(){
        return this.controller.get('tileUrl');
      }.property('tileUrl'),
    }),
    MarkerCollectionLayer.extend({
      content: function(){
        return [
          {location: this.controller.get('place').get('location')}
        ];
      }.property('location')
    })
  ]
});

I think this is the cleanest approach (fat models, skinny controllers). If for some reason I want all logic in my component (so no computed properties in the model or elsewhere) I can include a data function in my component:

data: function(){
  var x = this.get('place').get('lat');
  var y = this.get('place').get('lng');
    return {
      location: L.latLng(x, y)
    };
}.property('place')

Then I can call in the component:

this.get('data').location

Generally, note that it is this.get('place') in the component extension and this.controller.get('place') in the embedded extensions.

I can also set some model indepedant and/or more app specific stuff in a service that I inject in the application or in some routes:

//app/services/leafletService.js
import Ember from 'ember';

export default Ember.Service.extend({
  tileUrl: "url"
});

@miguelcobain
Copy link
Collaborator Author

How can you do this.controller.get('tileUrl'); in a component?
I thought a component was isolated from its controllers.
Quoting ember docs:

An Ember.Component is a view that is completely isolated. Properties accessed in its templates go to the view object and actions are targeted at the view object. There is no access to the surrounding context or outer controller; all contextual information must be passed in.

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

@miguelcobain I'm not sure about this. I think it is mainly a naming issue: The controller here is not the "real" controller, but refers to the scope of the component itself. Thus, if you extend TileLayer in the component, controller would be the components scope. Maybe there is also another way to make this work, but I haven't found it, yet. Do you know of any?

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

In other words: controller is not the outer controller, but the inner controller.

@miguelcobain
Copy link
Collaborator Author

Ah, probably some legacy code from when this was a view.

You probably can replace:

    TileLayer.extend({
      tileUrl: function(){
        return this.controller.get('tileUrl');
      }.property('tileUrl'),
    }),

with

    TileLayer.extend({
      tileUrl: this.get('tileUrl')
    })

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

No, that would give you:

`this` at the top level is undefined

You have to use it in a function. And then I could only make it work with controller, because the context has changed.

@csantero
Copy link

How about something like this?

/* global L */
import Ember from 'ember';
import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';
import MarkerCollectionLayer from 'ember-leaflet/layers/marker-collection';
import TileLayer from 'ember-leaflet/layers/tile';

var MyTileLayer = TileLayer.extend({
  tileUrl: Ember.computed.reads('leafletService.tileUrl')
});

var MyMarkerCollectionLayer = MarkerCollectionLayer.extend({
  content: Ember.computed('place.location', function () {
    return  [{ location: this.get('place.location') }];
  })
});

export default EmberLeafletComponent.extend({
  leafletService: Ember.inject.service('leaflet-service'),
  center: Ember.computed.reads('place.location'),
  tileLayer: Ember.computed('leafletService', function () {
    return MyTileLayer.create({
      leafletService: this.get('leafletService')
    });
  }),
  markerCollectionLayer: Ember.computed('place', function () {
    return MyMarkerCollectionLayer.create({
      place: this.get('place')
    });
  }),
  childLayers: Ember.computed.collect('tileLayer', 'markerCollectionLayer')
});
{{leaflet-map place=model}}
export default Ember.Route.extend({
  model: function() {
    return this.store.find('place');
  }
});

@csantero
Copy link

Updated my comment to fix the computed property in MyMarkerCollectionLayer.

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

@csantero that is also nice. I will try your way and see how I like it.

@miguelcobain
Copy link
Collaborator Author

Very smart things there.
But I really hate all this computed property madness. 😢

Edit: Don't like the service either. Seems like we're fighting ember/ember-leaflet.

@csantero
Copy link

@miguelcobain Could you elaborate?

@Awem
Copy link
Contributor

Awem commented Apr 27, 2015

Well, after looking into this I am also in favor of the Ember.computed syntax, because it doesn't depent on prototype extensions. Note that the syntax will change, because of emberjs/rfcs#11. You can try the new syntax via: https://github.com/rwjblue/ember-new-computed.
Regarding injecting the service into the component: in most cases I am not in favor of it, because it reduces reusability of the component. Think of:

{{leaflet-map tileUrl=leafletService.urlA}}
{{leaflet-map tileUrl=leafletService.urlB}}

No need to change anything in the component or use conditionals in it in order to render two maps with different tile sources.
Generally, I'm more and more leaning toward passing every single value via the template. I would only pass the whole model, if I want to do a lot with it in the component. So it would be like:

{{leaflet-map location=model.location name=model.name}}

This way you get leaner and more reusable components.
@csantero What is you reason for using create instead of extend?
@miguelcobain Yet another approach in your examples. Personally, I wouldn't do it like that, because you make heavy use of controllers. Ember is moving towards dropping controllers and using routable components instead (see emberjs/rfcs#15).

@miguelcobain
Copy link
Collaborator Author

Regarding injecting the service into the component: in most cases I am not in favor of it, because it reduces reusability of the component.

Exactly.

Ember is moving towards dropping controllers and using routable components instead

When routable components land, the controllers will become components, and the templates will become the components template. I see routable components as, mainly, non reusable components. But maybe I'm wrong. I believe migration will not be complicated. model is on controller just for illustration purposes.
Alternatively I could render a component like {{example1-block}} instead of {{render "example1"}}.
I'm open to alternatives.

@csantero
Copy link

@AW-UC I messed up: since I'm defining a computed property on TileLayer, I have to do that at extend-time. I'll edit my comment to fix that up.

@csantero
Copy link

Here is an example without the service:

/* global L */
import Ember from 'ember';
import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';
import MarkerCollectionLayer from 'ember-leaflet/layers/marker-collection';
import TileLayer from 'ember-leaflet/layers/tile';

var MyMarkerCollectionLayer = MarkerCollectionLayer.extend({
  content: Ember.computed('place.location', function () {
    return  [{ location: this.get('place.location') }];
  })
});

export default EmberLeafletComponent.extend({
  center: Ember.computed.reads('place.location'),
  tileLayer: Ember.computed('tileUrl', function () {
    return TileLayer.create({
      tileUrl: this.get('tileUrl')
    });
  }),
  markerCollectionLayer: Ember.computed('place', function () {
    return MyMarkerCollectionLayer.create({
      place: this.get('place')
    });
  }),
  childLayers: Ember.computed.collect('tileLayer', 'markerCollectionLayer')
});
{{leaflet-map place=model tileUrl=tileUrl}}
export default Ember.Route.extend({
  model: function() {
    return this.store.find('place');
  },
  setupController: function (controller, model) {
    this._super(controller, model);
    controller.set('tileUrl', 'http://example.com/some-tile-url');
  }
});

I'm not totally in love with re-creating the layer objects whenever 'tileUrl' or 'place' changes. It would be better to use the existing layer objects and then bind them to data. The alternative could be:

/* global L */
import Ember from 'ember';
import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';
import MarkerCollectionLayer from 'ember-leaflet/layers/marker-collection';
import TileLayer from 'ember-leaflet/layers/tile';

var MyTileLayer = TileLayer.extend({
  tileUrl: Ember.computed.reads('component.tileUrl')
});

var MyMarkerCollectionLayer = MarkerCollectionLayer.extend({
  content: Ember.computed('component.place.location', function () {
    return  [{ location: this.get('component.place.location') }];
  })
});

export default EmberLeafletComponent.extend({
  center: Ember.computed.reads('place.location'),
  tileLayer: Ember.computed(function () {
    return TileLayer.create({
      component: this
    });
  }),
  markerCollectionLayer: Ember.computed(function () {
    return MyMarkerCollectionLayer.create({
      component: this
    });
  }),
  childLayers: Ember.computed.collect('tileLayer', 'markerCollectionLayer')
});

Now the layers are bound directly to the data and are only instantiated once. Although passing this seems kind of ugly.

@Awem
Copy link
Contributor

Awem commented Apr 28, 2015

@csantero your approach of binding the layers directly to the data is interesting. I will have to try this to see how much of a difference it makes.
@miguelcobain you are right: refactoring will be easy. I think your examples work fine and do a good job of illustrating the most important things 👍

@miguelcobain
Copy link
Collaborator Author

Let me ask for your opinions.

Back in the days of Views, every view had a property controller set to the controller in which it was being rendered in.
Now, in Components, controller is set to the component instance itself, during initialization. See here: https://github.com/emberjs/ember.js/blob/v1.11.1/packages/ember-views/lib/views/component.js#L125

I was surprised that @AW-UC could access controller from child layers. I didn't make sense to me because Components don't have controllers per se, only Views do. However, he could do it just fine because when we create child layers, we also set its controller property to the parent's controller property. This is where that happens: https://github.com/gabesmed/ember-leaflet/blob/ember-cli-es6/addon%2Fmixins%2Fcontainer-layer.js#L120

That is what allows us to do things like:

export default LeafletMapComponent.extend({
  childLayers: [
    MarkerCollectionLayer.extend({
      //child layer holds `controller` property here
      content: Ember.computed.alias('controller.markers') 
    })
  ]
});

And this component was rendered from a template like:

{{leaflet-map markers=model}}

This is really cool. However, I don't think it makes sense to use the name controller anymore. In a previous comment, @csantero used a computed property "trick" to create child layers with a component property set to the parent component.
My opinion is that we should change the name of this property. Ember.Component docs don't even mention any controller property, and I believe Ember devs did that just for backwards compatibility reasons. At least it wasn't clear for me which controller we were referring to.

Any suggestions for the name of this property? After all, it is a very important API that people will certainly use, and it should be documented. Since @gabesmed was the original dev of this feature, he may have some suggestions to make. :)

  • map?
  • component?
  • container?
  • context? (Ember.Component also sets context to its own instance, just like it does with controller)
  • parent?

@csantero
Copy link

@miguelcobain dataSource?

@Awem
Copy link
Contributor

Awem commented Apr 29, 2015

I think map or mapData is nice. It is short and it makes clear, that it is something added by the addon.
BTW, while you can access data in the component itself through this.get('value') or Ember.computed.alias('value') you can alternatively also do this.get('controller.value') and Ember.computed.alias('controller.value'). If we rename controller we can suggest to use it everywhere for the sake of consistency. Than we would always use e.g.:

this.get('map.value')

and

Ember.computed.alias('map.value')

@miguelcobain
Copy link
Collaborator Author

@csantero dataSource looks like a connection to a database or something related with models. This is not necessarily something related with "data". This is a reference to the entire map component.

@csantero
Copy link

At the risk of derailing this into a pedantic discussion, I will argue that the layer objects shouldn't care whether the source of their data is the map component or something else. Passing the component instance to the layer is hardly elegant, but it is a convenient way to provide a bound data provider to the layer so that the layer can update its content. One could easily pass in something else to the layer that drives its content CP, and there's no reason that needs to be the map component. Abstracting that provider as "dataSource" seemed to make sense to me.

Of course if the compositional workflow you suggested in #98 (which I really love) ever becomes a reality, then this discussion becomes irrelevant.

@miguelcobain
Copy link
Collaborator Author

I completely agree with you.

I'm starting to think that #98 (comment) is what's left for a complete migration to the new ember standards...
Having these references and component subclassing around is a solution, but far inferior.

@Awem
Copy link
Contributor

Awem commented Apr 29, 2015

Using it somehow like #98 (comment) would definately be the most idiomatic way IMHO.
To open another topic: We are also in need for a strategy regarding Leaflet plugins. If that plugin adds a layer, then you can easily extend addon/layers/layer.js and acces controller from there. But if that plugin adds controls: how would you use it in a module? If you extend it, you can't access controller, because we miss the mixins.
For example, if I have http://www.liedman.net/leaflet-routing-machine/ installed I can make it work like this:

/* global L */
import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';

export default EmberLeafletComponent.extend({
  routingOptions: Ember.computed('origin','destination', function() {
    return {
      waypoints: [
        this.get('origin'),
        this.get('destination')
      ],
      fitSelectedRoutes: true
    };
  }),
  routingMachine: Ember.computed('routingOptions', function() {
    return L.Routing.control(this.get('routingOptions'));
  }),
  didCreateLayer: function() {
    this._super();
    this.get('routingMachine').addTo(this._layer);
  }
});

with {{show-route origin=model.origin destination=model.destination}} while origin and destination are of L.latLng();.
Now it would of course be nice, to import the routing part as a module. For this, we need to extend the API. Maybe create something like controlContainer. Maybe it is also possible to just use containerLayer for this. I haven't tried that, yet.

@miguelcobain
Copy link
Collaborator Author

Please provide an example of how you imagine it would be a better way to include that plugin/some control.
What you posted looks like a decent way to me.

@Awem
Copy link
Contributor

Awem commented Apr 29, 2015

@miguelcobain yes sure, it is totally sufficient. Would just be nice to import a module instead that can be reused. But I agree, that it is not the most pressing issue. I will try to present something better, once I got the time.

@Awem
Copy link
Contributor

Awem commented May 7, 2015

@miguelcobain Regarding your suggestion at #111 (comment) to use a component instead of {{render}}: I think that might be a little better, because not using render is more in accordance with the new Glimmer engine of Ember 1.13. However, this is certainly not that important right now.
For a third example, I would suggest something like http://leafletjs.com/examples/geojson.html. This is not too complicated and it would illustrate how to extend the component.

@gzurbach
Copy link

@miguelcobain I was wondering if the branch you created with the ember-leaflet component (ember-cli-es6) was in a PR somewhere? I'd love to use it but it is 134 commits behind and that does not seem very encouraging.
What would be awesome is to have it as a 1.0.0-beta or something so we would have a guarantee that there is no come back at this point.
I'm using ember-leaflet in production but I'm about to replace it with Leaflet and integrate it myself because I really want to use it in components. Unless your branch makes it to a beta release... ?
Ember 2.0 is supposed to be released in a month (June 12th?) and it'd be awesome to use ember-leaflet inside components!
Let me know if I am mistaken :)

@miguelcobain
Copy link
Collaborator Author

ember-cli-es6 is, quoting github:

This branch is 59 commits ahead, 134 commits behind master

This is expected because it was a complete rewrite from scratch. There are also 59 commits ahead.

You can start using the ember-cli branch using:

$ npm install --save-dev install gabesmed/ember-leaflet.git#ember-cli-es6
$ bower install --save leaflet leaflet.markercluster

When the branch lands on master/npm it should be really easy to migrate (just edit your package.json or run normal install).

@miguelcobain miguelcobain mentioned this pull request May 15, 2015
@gzurbach
Copy link

Got it, that makes sense now. Do you have an idea of when this branch will make it to master? I'm gonna try to integrate it this weekend in my app and let you know if I have feedback. Thanks!

@miguelcobain
Copy link
Collaborator Author

Thank you!
For me it will be merged on the example page is done.

@saerdnaer
Copy link

I'm gonna try to integrate it this weekend in my app and let you know if I have feedback.

@gzurbach How did it go?

@gzurbach
Copy link

I have not been able to do it yet unfortunately. I really need to anyways because the current implementation with view is causing a lot of issues. I will get back you you guys as soon as I have something.

@gzurbach
Copy link

I'm working now on moving my map to use the ES6 branch. The main issue I'm having right now is that I can't figure out how to add a layer to a cluster.
I'm using the marker-cluster layer (https://github.com/gabesmed/ember-leaflet/blob/ember-cli-es6/addon/layers/marker-cluster.js).

The code i'm trying to migrate does the following:

var markers = new L.MarkerClusterGroup();
var geoJsonLayer = new L.geoJson();
geoJsonLayer.addData(data);
markers.addLayer(geoJsonLayer);

Which I translated to something like:

import EmberLeafletComponent from 'ember-leaflet/components/leaflet-map';
import MarkerClusterLayer from 'ember-leaflet/layers/marker-cluster';
import TileLayer from 'ember-leaflet/layers/tile';

var tileLayer = [...]
var markerClusterLayer = MarkerClusterLayer.extend();

export default EmberLeafletComponent.extend({
    childLayers: [
        tileLayer,
        markerClusterLayer
    ],
    markersDidChange: function () {
        var markers = this.get('controller.markers');
        var geoJsonLayer = new L.geoJson();
        geoJsonLayer.addData(markers);
        markerClusterLayer.addLayer(geoJsonLayer);
    }.observes('controller.markers')
});

I am getting: markerClusterLayer.addLayer is not a function

Sorry if my question is silly. I'm still new to Leaflet. Any suggestions?

@miguelcobain
Copy link
Collaborator Author

Tests are often a good source of examples.
Check marker cluster tests: https://github.com/gabesmed/ember-leaflet/blob/ember-cli-es6/tests/unit/layers/marker-cluster-test.js

@gzurbach
Copy link

Thanks for pointing out the tests. If I understand correctly, it seems like I cannot use MarkerClusterLayer without MarkerCollectionLayer? I'm not sure I understand where the GeoJson layer should be then? As a child of the MarkerCollectionLayer which itself is a child of MarkerClusterLayer?

@gabesmed gabesmed closed this Oct 14, 2015
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.

None yet

6 participants