-
Notifications
You must be signed in to change notification settings - Fork 821
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
Add OverlappingMarkerSpiderfier package #1329
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
I added some comments. We should also change the naming a bit:
Package name: @agm/marker-spiderfier
Folder name: marker-spiderfier
/** | ||
* Triggers when a marker is formatted, can be used to style the icon | ||
*/ | ||
@Output() format: EventEmitter<{ marker: Marker, status: string }> = new EventEmitter<{ marker: Marker, status: string }>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract this as a type? So others can use this in their code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
/** | ||
* Triggers when markers are spiderfied (expanded) | ||
*/ | ||
@Output() spiderfy: EventEmitter<{ changedMarkers: Marker[], unchangedMarkers: Marker[] }> = new EventEmitter<{ changedMarkers: Marker[], unchangedMarkers: Marker[] }>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
packages/spiderfier/README.md
Outdated
|
||
----- | ||
|
||
this package levereges the [npm-overlapping-marker-spiderfier][npm-overlapping-marker-spiderfier] to add spiderfy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this -> This
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
*/ | ||
@Directive({ | ||
selector: 'agm-marker-spider', | ||
providers: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as want to start implementing ChangeDetectionStrategy.OnPush for all component: can you add it here and test that it runs with OnPush?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changeDetection attribute is only available for components, not directives. Please update the @agm/core
package and I'll test if this gives any problems in my package
/** | ||
* Triggers when markers are unspiderfied (collapsed) | ||
*/ | ||
@Output() unspiderfy: EventEmitter<{ changedMarkers: Marker[], unchangedMarkers: Marker[] }> = new EventEmitter<{ changedMarkers: Marker[], unchangedMarkers: Marker[] }>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here for the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
packages/spiderfier/README.md
Outdated
@agm/js-marker-spider has a peer depedency on [npm-overlapping-marker-spiderfier][npm-overlapping-marker-spiderfier] | ||
|
||
```shell | ||
npm install npm-overlapping-marker-spiderfier @agm/js-marker-spider --save |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use a version range here for npm-overlapping-marker-spiderfier
that works with our package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually made a full ts rewrite of the package that is 100% compatible but forgot to update the readme, I will update and include version info
packages/spiderfier/README.md
Outdated
```shell | ||
npm install npm-overlapping-marker-spiderfier @agm/js-marker-spider --save | ||
# or | ||
yarn add npm-overlapping-marker-spiderfier @agm/js-marker-spider |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here also
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
|
||
const markerPromise = this._markerManager.getNativeMarker(marker); | ||
|
||
this._markers.set(marker, markerPromise); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens in the _makerManager.addMarker already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but the _markerManager is a reference to the parent (notice the @SkipSelf) which means that the local _markers is not updated. By setting the _markers here too, it allows for the manager to keep a local reference to it's own markers which is used in removal/clear to only delete markers owned by the spiderfier, not by any parent (cluster/agm-map) managers)...
The update(s) to core (as explained in the intro) are to allow cascading of marker creation from "nested" marker managers up the chain (until the "core" marker manager belonging to the "agm-map" directive, that way it is possible to have a clustered-spiderfier (or any other nested (grouping) marker layer)...
if (this._markers.has(marker)) { | ||
spider.removeMarker(nativeMarker as any); | ||
|
||
this._markers.delete(marker); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This happens in the _makerManager.addMarker already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
} | ||
|
||
createEventObservable<T>(eventName: string, marker: AgmMarker): Observable<T> { | ||
// Override the default "click" event with "spider_click" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be a valid use case that the user want's a click event instead of a spider_click event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The click event in the spiderfier is not very functional since it will also trigger when the "root" marker is clicked. Long story short: the click event might be usefull for only very few people and to (properly) support the info window, I have to replace the click event (otherwise the infowindow will also trigger when clicking the still collapsed markers)...
If you (or anyone) has a suggestion I'm all ears...
I pushed a commit with most of the changes, please comment on the few ones remaining with questions and I'll modify and commit accordingly... |
@SebastianM the requested replacements of |
Example of nested cluster and spiderfier (this comes from my own (working) test component): <agm-map [latitude]="51.673858" [longitude]="7.815982" [maxZoom]="16">
<agm-marker-cluster [maxZoom]="14"
imagePath="https://raw.githubusercontent.com/googlemaps/v3-utility-library/master/markerclustererplus/images/m">
<agm-marker-spider [keepSpiderfied]="true" (format)="testEvent('format', $event)"
(spiderfy)="testEvent('spiderfy', $event)" (unspiderfy)="testEvent('unspiderfy', $event)">
<agm-marker [latitude]="51.673858" [longitude]="7.815982"
(markerClick)="testEvent('markerClick', $event)"></agm-marker>
<agm-marker [latitude]="51.673858" [longitude]="7.815982"
(markerClick)="testEvent('markerClick', $event)"></agm-marker>
<agm-marker [latitude]="51.673858" [longitude]="7.815982"
(markerClick)="testEvent('markerClick', $event)"></agm-marker>
<agm-marker [latitude]="51.673858" [longitude]="7.815982"
(markerClick)="testEvent('markerClick', $event)"></agm-marker>
<agm-marker [latitude]="51.67" [longitude]="7.8"
(markerClick)="testEvent('markerClick', $event)"></agm-marker>
</agm-marker-spider>
</agm-marker-cluster>
</agm-map> |
- Replace google.maps types with corresponding @agm types
@SebastianM any update on this? |
@SebastianM are there any updates on this? |
@SebastianM Any updates on this? |
<agm-marker-cluster [maxZoom]="14" this not working for me please help me @chancezeus |
@SebastianM are there any updates on this? |
Hola ! Any update on this ? Thanks ! |
@SebastianM Any updates on this? |
@chancezeus I have had to use this feature so I took the liberty to publish a package. If anyone feel interested to, follow link of the package. |
You sir are my hero 4 the day! |
error agm-marker-spider is not known element |
@drjaat you import agm-oms in same location where you imports @agm/core? And you export AgmMarkerSpiderModule? If you do not try export this module. |
Does this play well with marker clusterer ? If so can you update the README with the syntax please |
I tested it and found that it does |
Thanks for the reply. When using it in my setup I have a clustered marker of 2 and when I click on it, it just flashes and doesn't separate. Here is my mark up:
Is this correct ? I don't get any errors. |
turn clustering off at a certain zoom point using [maxZoom]="10" I changed your code to as below code below
|
Works a treat ! Thank you |
This extension works great! Just one question: How do I use the |
I'm getting the error: I think it's a typo isn't it? Because spider-manager.js tries to call |
Someone fixed it in a different version of the package: https://www.npmjs.com/package/agm-spiderfeir |
This extension saved my days. Thank you for your great work. Hope this fully merged to main branch. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
I met problems when trying to use agm-oms with js-marker-clusterer. Hopfully, the package of marker spiderfier can be fully supported in agm asap. Thank you very much. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
when this will be completed? :) |
Whenever someone adopts this PR and makes the necessary changes. I won't be doing it anytime soon, since there are more pressing issues. |
I resolved conflicts, rebuild the angular-spiderfier plugin and published the new package under https://www.npmjs.com/package/agm-spiderfier Now, I don't know if it's better to keep this package distinct or merge it in agm. |
|
It work perfect! |
well, good point, it works under https://www.npmjs.com/package/agm-spiderfier1 with latest agm/core |
but when I try to take the production build, I am getting the error : Module not found: Error: Can't resolve 'agm-spiderfier' |
I had the same problem and to solve it follow this: Install as dev Dependencies
Install as Dependencies
At your tsconfig.app.json{
"extends": "./tsconfig.json",
"compilerOptions": {
"outDir": "./out-tsc/app",
// Add types jest
"types": ["jest"]
},
"include": [
"src/**/*.ts",
// Add agm-spiderfier to be compiled
"./node_modules/agm-spiderfier/**/*.ts"
],
"exclude": ["src/test.ts", "src/**/*.spec.ts"]
} |
replaced by #1771 |
not work !
|
With this nested approach of cluster > spider, is there a way that when cluster has clicked, then it will spiderfy after? Thanks |
As mentioned in #1146 it would be nice to have support for the OverlappingMarkerSpiderfier. Since a lot of (if not most) people would also like to be able to combine the spiderfier with the cluster library I made some changes to the way the
MarkerManager
works. The change basically makes it possible forMarkerManager
s to be nested which enables the spider and cluster plugins to be active at the same time (ordering should not matter) and forwarding the actual creation of markers onto the "parent"MarkerManager
(this will be theMarkerManager
instance associated with theAgmMap
instance). I did not want to modify too much in the core packages, but I did extend the baseMarkerManager
constructor with an@Optional() @SkipSelf() _markerManager: MarkerManager
. If interested I can add extra functionality to also have the "core"MarkerManager
forward it's calls up the tree (until the "parent" is not defined anymore) and thus add the option to create for example anAgmMarkerGroup
directive that clusters similar markers which can be disabled at once and/or other (more advanced) features...