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

export Leaflet object, public components and toLatLng macro #141

Merged

Conversation

buschtoens
Copy link
Contributor

@buschtoens buschtoens commented Mar 23, 2017

Accessing the Leaflet object (L) is currently only possible like so:

const L = window.L;

Whereas this PR enables importing Leaflet from the addon package.

import L from 'ember-leaflet';

When #106 lands this obviously would need adaption.

@miguelcobain
Copy link
Owner

miguelcobain commented Mar 23, 2017

@buschtoens I think the way forward would be to

import L from 'leaflet';

directly. However, es6 imports from npm modules with ember-cli doesn't seem to be very easy at the moment.
I don't mind to export L from ember-leaflet, But I think using the default export might be too much.

A snippet from react-leaflet:

import { Map, Marker, Popup, TileLayer } from 'react-leaflet';

Would you mind changing index.js to export all leaflet layers + L so that we have a similar thing in ember-leaflet? That would be awesome.

@buschtoens buschtoens force-pushed the export-leaflet-as-default branch from 903b461 to 9f9a59b Compare March 23, 2017 18:05
@buschtoens
Copy link
Contributor Author

I've exported all classes listed in the reference.

@miguelcobain
Copy link
Owner

@buschtoens there was a communication error, I think.
I was asking to export all the ember-leaflet classes, not the leaflet ones. Those can be found in the leaflet package.

@buschtoens
Copy link
Contributor Author

buschtoens commented Mar 23, 2017 via email

@buschtoens
Copy link
Contributor Author

Just for clarification: Which files do you want exported?

  • components
  • helpers
  • macros
  • mixins

And what would be the benefit? Couldn't they be imported like so?

import BaseLayerComponent from 'ember-leaflet/components/base-layer';
import { latLngBounds } from 'ember-leaflet/helpers/lat-lng-bounds';
import toLatLng from 'ember-leaflet/macros/to-lat-lng';
import DraggabilityMixin from 'ember-leaflet/mixins/draggability';

Or is this merely for easier access?

@miguelcobain
Copy link
Owner

Let's start with public-ish things.

Components:

  • CircleLayer

  • CircleMarkerLayer

  • GeojsonLayer

  • ImageLayer

  • LeafletMap

  • MarkerLayer

  • PolygonLayer

  • PolylineLayer

  • PopupLayer

  • TileLayer

  • TooltipLayer

  • WmsTileLayer

  • toLatLng macro

I don't see a need to export helpers here. They're basically useless outside of templates. The user could just use the L.method directly instead.
I don't feel very confident exporting mixins right now. They're something private to share functionality between some layers, just like some components.

And what would be the benefit? Couldn't they be imported like so?
Or is this merely for easier access?

Well, yes, at the moment they would be just for easier access because ember-cli doesn't have a way for addons to have private components.
I think this API has some benefits:

  • when ember-cli's module unification RFC gets implemented, we will have such a thing as private components
  • this API is more in line with es6 module standards
  • at least we have some decision on what to export or not, unlike the import from 'ember-leaflet/components/*' form

We should use es6 reexport shorthand like so:

export { default as CircleLayer } from 'ember-leaflet/components/circle-layer';
export { default as CircleMarkerLayer } from 'ember-leaflet/components/circle-marker-layer';
export { default as GeojsonLayer } from 'ember-leaflet/components/geojson-layer';
// ...

@buschtoens buschtoens force-pushed the export-leaflet-as-default branch from 9f9a59b to 544c45b Compare March 24, 2017 11:40
buschtoens added a commit to buschtoens/ember-leaflet that referenced this pull request Mar 24, 2017
@buschtoens buschtoens force-pushed the export-leaflet-as-default branch from 544c45b to e0fe57b Compare March 24, 2017 11:41
buschtoens added a commit to buschtoens/ember-leaflet that referenced this pull request Mar 24, 2017
@buschtoens
Copy link
Contributor Author

Done. 🏁

Changelog has been amended. 📝

@buschtoens buschtoens changed the title export Leaflet object as default export Leaflet object, public components and toLatLng macro Mar 24, 2017
addon/index.js Outdated
@@ -0,0 +1,30 @@
const L = window.L || {};

export default L;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel enough confidence to use the default export yet.
It would make sense for the leaflet package itself, but we may want to use this export for something else later?

addon/index.js Outdated

export { default as CircleMarkerLayer } from 'ember-leaflet/components/circle-marker-layer';

export { default as GeojsonLayer } from 'ember-leaflet/components/geo-json-layer';
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

geo-json-layer file doesn't exist.

@buschtoens buschtoens force-pushed the export-leaflet-as-default branch from e0fe57b to 8e75083 Compare March 24, 2017 12:07
@buschtoens
Copy link
Contributor Author

buschtoens commented Mar 24, 2017

  • Removed default export
  • Fixed GeojsonLayer path

@miguelcobain
Copy link
Owner

🎉

@miguelcobain miguelcobain merged commit ad5611f into miguelcobain:master Mar 24, 2017
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.

2 participants