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

Bindings for "bounds", "style", other props in mapbox-gl #32

Closed
allthesignals opened this issue Oct 3, 2017 · 5 comments
Closed

Bindings for "bounds", "style", other props in mapbox-gl #32

allthesignals opened this issue Oct 3, 2017 · 5 comments

Comments

@allthesignals
Copy link
Contributor

allthesignals commented Oct 3, 2017

I'm running into a use case where we manage styles entirely through setStyle of mapbox-gl Map class. This lends itself to some intense actions that fetch and set a style object using a reference to the map instance's setStyle method. This is probably okay, but I can see an argument for a few mapbox-gl-level properties that manage setStyle and fitBounds internally. Using @miguelcobain's Ember Leaflet as an analog, you can see the mapping of properties to Leaflet's internal methods under the "bound options" section:

zoom (uses setZoom())
center (uses panTo())
maxBounds (uses setMaxBounds())
bounds (uses fitBounds())

I know that map.call works for these cases - {{map.call 'fitBounds' bounds}} {{map.call 'setStyle' styleObject}} - but it seems awkward to think of calling functions (as opposed to actions) inside of templates where we should be passing actions and properties. This introduces some non-MapboxGL properties to our API, but I don't think they will collide.

The "Promise aware" part of the title is a nice-to-have - to be able to pass promises down and have components resolve them would be great, but I can imagine having that in a generic API would make it impossible to handle things like failed promises and so on...

I wanted to get your thoughts on this and maybe I could start a branch.

Thank you!

@miguelcobain
Copy link

miguelcobain commented Oct 3, 2017

IMO, I think that invoking helpers/components on templates for other purposes other than actually rendering things is an anti pattern.

@kturney
Copy link
Owner

kturney commented Oct 3, 2017

I agree that binding properties to the map feels more embery, but I'm trying to balance that with exposing the full mapbox-gl-js API. Currently, the templates read mostly like one was using the map directly, except with auto-binding to call methods when data changes.

I also worry about handling these property to method mappings and then mapbox changing their API, for example unifying the camera API.

Maybe a subset of properties to methods could be added, but then it's a crap shoot for people knowing when they can use a binding and when they need to fall back to map.call.

The bindings feel like a feature that will be easier to add post mapbox-gl-js 1.0.

As for being promise aware, I'm not sure that ember-mapbox-gl is the right place to handle that. Perhaps some ember-concurrency tasks with task.lastSuccesful.value bound to the style and bounds would solve your use case?

@allthesignals allthesignals changed the title Promise-aware bindings for "bounds", "style", other props in mapbox-gl Bindings for "bounds", "style", other props in mapbox-gl Oct 3, 2017
@allthesignals
Copy link
Contributor Author

Gotcha. I'm not sure how much the "bounds" and "style" parts of their API will change or has changed, though, but as you've mentioned before, mapbox-gl-js is a little volatile right now. In my experience, pre- and post-Leaflet 1.0 did not have a tremendous impact on general map position state APIs, so I think ember-leaflet was able implement this property-method binding pattern pre-1.0 (though @miguelcobain could probably speak more to that).

Point taken on confusion re: falling back to map.call. mapbox-gl-call's auto-binding takes care of keeping it as Ember-y as possible. I might argue that perhaps these properties are available still on mapbox-gl but use mapbox-gl-call internally (in the template) if they exist.

some-consumer-template.hbs:

{{mapbox-gl bounds=myBoundsComputedProp}}

mapbox-gl.hbs:

{{#if glSupported}}
  {{#if map}}

    {{#if bounds}}
       {{mapbox-gl-call 'fitBounds' bounds obj=map}}
    {{/if}}

    {{yield (hash
      call=(component 'mapbox-gl-call' obj=map)
      control=(component 'mapbox-gl-control' map=map)
      image=(component 'mapbox-gl-image' map=map)
      layer=(component 'mapbox-gl-layer' map=map)
      marker=(component 'mapbox-gl-marker' map=map)
      on=(component 'mapbox-gl-on' eventSource=map)
      popup=(component 'mapbox-gl-popup' map=map)
      source=(component 'mapbox-gl-source' map=map)
    )}}
  {{/if}}
{{else}}
  {{yield to='inverse'}}
{{/if}}

I could probably just do this locally in my own project though ¯_(ツ)_/¯.

Agree on promise-awareness - it was just a thought. E-concurrency or promise helpers are the right path for that!

Thanks!

@kturney
Copy link
Owner

kturney commented Oct 3, 2017

hmmmm. I'm also not sure how in that scenario one could pass in options and eventData to fitBounds.

@miguelcobain
Copy link

miguelcobain commented Oct 4, 2017

What I did on ember-leaflet was to create a sort of DSL for binding properties, much like we have for classNameBindings.
Consider this example on the leaflet-map component:

leafletProperties: [
  'zoom:setZoom:zoomPanOptions', 'minZoom', 'maxZoom',
  'center:panTo:zoomPanOptions',
  'bounds:fitBounds:fitBoundsOptions', 'maxBounds'
]

from https://github.com/miguelcobain/ember-leaflet/blob/master/addon/components/leaflet-map.js#L42-L46

For example, 'zoom:setZoom:zoomPanOptions', means "watch for changes in the zoom property and call the setZoom method with zoomPanOptions as an argument.

Of course, leaflet breaking changes will always be tied to ember-leaflet breaking changes, but I think that is somehow expected. However, using these sort of dynamic "tricks" we can easily accommodate for simple simple api changes.
It's a tradeoff, I guess. The main focus of ember-leaflet was to make ember developers "feel at home".

@kturney kturney closed this as completed May 6, 2020
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