-
Notifications
You must be signed in to change notification settings - Fork 48
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 some additional callback options #21
base: master
Are you sure you want to change the base?
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.
Appreciate the PR. It would be better to return the spiderLegs on spiderfy
and unspiderfy
methods. Thereby the implementation/caller can generate events or do other things based on it rather that put it in the library.
KeyUp and the readme changes look good.
index.js
Outdated
) | ||
} | ||
|
||
options.onSpiderify(previousSpiderLegs) |
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 would suggest, we return the spiderLegs on spiderfy
and unspiderfy
method calls. Since they are called from outside the library and i do not understand the need to emit these as events. For example:
spiderifier.onSpiderfy = (args) => {
....
}
spiderifier.spiderfy([-74.50, 40], features);
const spiderLegs = spiderifier.spiderfy([-74.50, 40], features);
onSpiderfy(spiderLegs);
clusterElement.dispatchEvent(
new CustomEvent('spiderify', {
spiderLegs
})
);
The clusterElement seems like a implementation specific one and not generic for the library.
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.
Totally up to you. Yes the clusterElement implementation was because, in the particular project, we wanted to manipulate how the original cluster marker looked when it was spiderified vs not. But I can remove for the library.
I'm not sure I quite understand what you're asking in suggesting returning the spiderLegs. But I attempted to adjust.
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.
Return the spiderLegs in the spiderfy and unspiderfy function in the library. That way when you call spiderfy or unspiderfy, you can get the legs and do what you want with it
We really like your library. I added a few callbacks and custom events for spiderifying/unspiderifying that we needed for a project. Thought I would submit a PR with our changes in case anyone else may benefit from it.