Skip to content
This repository was archived by the owner on Oct 6, 2023. It is now read-only.

Conversation

@jgravois
Copy link
Contributor

@jgravois jgravois commented Jun 20, 2020

relies on Esri/esri-leaflet-cluster#31
resolves #156

while i was in there I also:

  • started importing the ES modules from Leaflet and Esri Leaflet directly (good for webpack users etc.)
  • moved all the pages we created back in the day for visual QA/QC out of /spec and into /debug so they'd be easier for folks to find.

the only thing i don't like about this approach is that AMD loaders like Dojo and RequireJS now require that the cluster plugin be loaded whether its needed or not.

neither are a popular option outside of JSAPI land these days though. i can live with the extra cruft since folks using modern bundlers or the UMD won't have to worry about it.

the diff is a bit of a mess, so don't be shy if you have any questions.

var oldUnbindPopup = Util.bind(this.unbindPopup, this);
var oldOnRemove = Util.bind(this.onRemove, this);

L.Util.bind(this.createNewLayer, this);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

right now this throws an error. fixed by Esri/esri-leaflet-cluster#31.

i couldn't find a layer to test with, but it looks like this.createNewLayer is only really useful for displaying polygon services rendered using proportional marker symbols.

@gavinr
Copy link
Contributor

gavinr commented Jun 22, 2020

Thanks @jgravois! I've merged Esri/esri-leaflet-cluster#31 and released under v2.1.0, will test/merge/release this PR soon.

@gavinr gavinr self-requested a review June 22, 2020 19:50
Copy link
Contributor

@gavinr gavinr left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for the additional debug/sample pages. The cluster one is now working after I updated it to use [email protected].

@jgravois
Copy link
Contributor Author

very cool!

@gavinr gavinr merged commit c3ade3c into Esri:master Jun 23, 2020
@gavinr
Copy link
Contributor

gavinr commented Jun 23, 2020

These fixes were released in https://github.com/Esri/esri-leaflet-renderers/releases/tag/v2.1.1. Thanks again @jgravois

@francharbo
Copy link

Sorry to reopen this, maybe I should create a bug ?

I tried to use v2.1.1 using import style declaration for the plugins :

import L from 'leaflet'
import * as esri from 'esri-leaflet'
import * as Cluster from 'esri-leaflet-cluster'
import 'leaflet.markercluster/dist/leaflet.markercluster'
import MarkerClusterCss from 'leaflet.markercluster/dist/MarkerCluster.css'
import DefaultMarkerCss from 'leaflet.markercluster/dist/MarkerCluster.Default.css'

L.esri = esri
L.esri.Cluster = Cluster

import 'esri-leaflet-renderers'

But the initHook aren't attached to esri and esri.Cluster featureLayer.

It does work with v2.1.0 though.
Any idea of how to solve this ?

@jgravois jgravois deleted the fix-cluster branch June 24, 2020 14:38
@jgravois
Copy link
Contributor Author

Any idea of how to solve this?

off the top of my head no, but if you're using import statements, why not go whole hog and just import the ES Modules you're using?

import { map } from 'leaflet'
import { featureLayer } from 'esri-leaflet'

either way if you're able to provide a complete, self-contained, simplified, ready to run repro case instead of just a code snippet, it'd speed up the debugging process.

@francharbo
Copy link

So I reproduced a minimal example of how I build my project.
However, I didn't reproduce what I see on my project...

I'm very sorry I disturbed you, my problem seems to rely in the complexity of my project

Mea culpa

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Uncaught TypeError: Cannot read property 'options' of undefined

4 participants