miguelcobain / ember-leaflet

:fire: :leaves: Easy and declarative mapping for ember
https://miguelcobain.github.io/ember-leaflet/
MIT License
210 stars 87 forks source link

write cookbook section and refer inheritance style ember-leaflet #11

Closed digitalcora closed 5 years ago

digitalcora commented 8 years ago

From what I gather, this should be a supported way of implementing event handlers:

// app/components/custom-map.js
import LeafletMap from 'ember-leaflet/components/leaflet-map';

export default LeafletMap.extend({
  wasDragged: false,
  onDragstart() {
    this.set('wasDragged', true);
  }
});

When I do this and start dragging the map, I get a "this" is undefined error. And indeed if I check in the debugger, this has no value and there seems to be no way to reach the component instance from the event handler.

I tried setting some breakpoints within the _addEventListeners code in base-layer, but no matter where I checked, this was correctly pointing to the component instance. I couldn't figure out how to proceed from there; it doesn't look like there are any tests in the project that cover this functionality.

miguelcobain commented 8 years ago

From what I gather, this should be a supported way of implementing event handlers

  1. Why do you say that? What part of the docs made you think that?
  2. Are you trying to catch dragstart of a map or of a marker?
  3. Can you show me what the template looks like?
digitalcora commented 8 years ago

Why do you say that? What part of the docs made you think that?

The functionality appears undocumented aside from this comment in the source, and the fact that adding a function with the correct name does in fact result in the function getting called.

I was investigating this rabbit hole because I need the event handlers to be located on my custom map component itself, which is where the relevant property is located (wasDragged in my simplified example), and not on the higher-level component or controller that is invoking the component. It's entirely possible that there is another way to do this I've overlooked – I'm open to suggestions :smile:

Are you trying to catch dragstart of a map or of a marker?

The map.

Can you show me what the template looks like?

The templates are like this (very simplified):

{{!-- app/templates/map-view.hbs --}}
{{custom-map user=currentUser things=currentThings}}
{{!-- app/templates/components/custom-map.hbs --}}
{{tile-layer url='...'}}
{{marker-layer location=user.location}}
{{#each things as |thing|}}
  {{marker-layer location=thing.location}}
{{/each}}

Combine with the JS above and you see the full picture.

miguelcobain commented 8 years ago

@grantovich You're sneaky. :stuck_out_tongue_winking_eye: That functionality isn't public.

If you take a closer look, that code calls a function named methodName which is defined as '_' + eventName. So, basically if you implement a method called _dragstart it should give you the functionality you want. This was meant to be used privately by ember-leaflet.

The reason we can't use onDragstart is because that name is reserved for actions. I'm open to suggestions, but basically this is a matter of naming. I'm not opposed to make this functionality documented.

digitalcora commented 8 years ago

Haha, thanks! I should indeed have taken a closer look – I thought "well clearly since the function is being called at all, that ember-leaflet code must be calling it", without remembering that Ember itself has that built-in event handling. I had thought it was without the "on", though – just click works, for example, rather than onClick. I wonder if that difference is the reason this wasn't set correctly.

It seems like there are two possible "styles" of using ember-leaflet: Keeping all actions, custom layer properties, and state tracking in a controller/component whose template contains the leaflet-map (the style the docs use), and having things located in the component they apply to (the style I'm using). IMO the latter approach has a major separation-of-concerns advantage once you get to a non-trivial level of complexity, and more closely aligns with Ember conventions. Right now I have 5 custom map components (markers, polyline layers, etc.) with an average of 30 LOC each – I could combine them all into a 150-line mega-controller, but that would be kind of defeating the point of using components. Additionally, this approach enables the use of this._layer to access the underlying Leaflet object, in case the built-in property bindings are not sufficient.

If you're using this approach, of course, it becomes necessary to have components be responsible for handling their own events, which is how I arrived at this "hidden feature" :smile:

I'm open to suggestions, but basically this is a matter of naming. I'm not opposed to make this functionality documented.

Perhaps layerDragstart or onLayerDragstart? leafletDragstart?

I'd be open to writing an addition to the docs that describes this custom-components way of doing things, though off-hand it looks like it would require some structural changes since the current example format assumes that all JS is in one file.

miguelcobain commented 8 years ago

Generally, I agree with your points.

IMO the latter approach has a major separation-of-concerns advantage once you get to a non-trivial level of complexity, and more closely aligns with Ember conventions.

I don't really agree with this. For example, I have a component called map-with-controls with renders a {{#leaflet-map and some custom layer controls I made using regular HTML. I can use map-with-controls anywhere in my app, as it is self contained, just as a component should be.

You're basically arguing in favor of inheritance over composition. Definitely both use cases are valid.

What I like about ember-leaflet is that, if you think, all of this discussion still applies to regular components. They are no different from them. So, we take advantage of the communities thoughts and patterns, just like any other component (despite being a bit "special" since they don't render DOM).

Perhaps layerDragstart or onLayerDragstart? leafletDragstart?

I think that following Ember.Component's conventions would be better. Why not just the event name? When we want to handle a click event, we just implement a click method. Similarly, when we want to handle a dragstart we could just implement dragstartevent.

I'd be open to writing an addition to the docs that describes this custom-components way of doing things, though off-hand it looks like it would require some structural changes since the current example format assumes that all JS is in one file.

I'm willing to add a quick note about this, probably in the "Actions and interaction" section. Something like "if you inherit an ember-leaflet class, you can implement methods leaflet event names to handle the event". This is actually similar to how ember-leaflet 1 worked and would be nice to have.

I still think that the docs take a good approach for people starting. Any experienced ember developer knows that using a Controller is no different than using a Component, in those examples. Favoring inheritance also implies, I think.

What I would definitely want would be a section of the website similar to this one: a cookbook http://www.ember-power-select.com/cookbook

"System-wide config" still applies here. A recipe talking about inheritance in ember-leaflet development would also be very nice.

I'll reopen this to remind me of these additions. PR's welcome!

digitalcora commented 8 years ago

I don't really agree with this. For example, I have a component called map-with-controls with renders a {{#leaflet-map and some custom layer controls I made using regular HTML. I can use map-with-controls anywhere in my app, as it is self contained, just as a component should be.

I see what you mean, I think. So your suggestion, if I wanted a custom marker (and wanted to keep that custom marker logic isolated in its own component), would be to create a thing-marker component, put the logic in there, and have that component's template just be a one-line {{marker-layer ...}}?

I'm not sure I see the advantage over my current approach, since the end result is basically the same but plus one extra template file per component to think about, and you don't have this._layer in case you need direct access to the Leaflet layer object (e.g. for functions like zoomIn() that have no direct analogue in the property bindings). I can see where you're coming from, though.

I think that following Ember.Component's conventions would be better. Why not just the event name? When we want to handle a click event, we just implement a click method. Similarly, when we want to handle a dragstart we could just implement dragstart event.

Why not, indeed! I didn't realize this was possible, assuming it would cause problems with Ember's built-in event handling. If it will actually work then that sounds like the most intuitive option :smiley:

miguelcobain commented 8 years ago

@grantovich In my case I need to encapsulate not just the leaflet map but also some adjacent HTML that I then position absolutely over the map with CSS. Example:

<div class="controls">
  <ul>
    <li>{{input type="checkbox" value="satelliteEnabled"}} Satellite tiles</li>
    <li>{{input type="checkbox value="roadmapEnabled"}} Roadmap tiles</li>
    <li>{{input type="checkbox value="otherEnabled"}} Other tiles</li>
  </ul>
</div>

{{#leaflet-map center=center zoom=zoom}}
  {{!-- layers here --}}
{{/leaflet-map}}

I couldn't do this with inheritance. I also use inheritance for some layers. Like I said, both are useful.

e.g. for functions like zoomIn() that have no direct analogue in the property bindings

The analogue would be this.incrementProperty('zoom'). I prefer this because if switching map implementations would leave this untouched. The least I use leaflet API, the better. Of course sometimes that's not possible or impractical.

Why not, indeed! I didn't realize this was possible, assuming it would cause problems with Ember's built-in event handling. If it will actually work then that sounds like the most intuitive option :smiley:

I realized that we can't probably do it because it would clash with existing events like click. I was being silly.