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

a couple of changes #15

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

a couple of changes #15

wants to merge 18 commits into from

Conversation

kwibus
Copy link
Contributor

@kwibus kwibus commented Jun 7, 2017

made a couple of changes, and maybe they can be useful for you.
It's now 1 single pull request, but i can split it up if you interesserend.
the change:

  • edgeMarker points to single coordinate/marker(with bindEdgeMarker) like leaflet tooltip

  • rotation if marker is done via leaflet-rotatedmarker plugin

  • location marker is different (more correct). i
    in old version it is has x and y values as close as possible to target
    now it lies on the intersection of edge and line from mid screen to target

  • you pass other border then map window. like Leaflet-active-area

  • has package.json for npm (needs to be updated if you use this)

  • it removes some options:

    • distanceOpacity: false
    • distanceOpacityFactor
    • layerGroup: null,
    • rotateIcons: true,
    • iconUrl: L.Icon.Default.imagePath + '/edge-arrow-marker.png',

    but the can be implementer easily.

hope you are interested, comments are welcome. it try to learn javascript

@ubergesundheit
Copy link
Owner

Hi, thanks for your efforts!

I think I would prefer seperate pull requests for each change. Then please explain why you've made the changes. Also, please include documentation and examples in the index.html for each change.

I couldn't make out any glaring javascript mistakes, so from this side it should be good.

Cheers

@kwibus
Copy link
Contributor Author

kwibus commented Jun 8, 2017

I am glad to help, hope these pull request help.
It was a good excuse to look at the code again.
some notes and thinks i did not include:

  • have not tested it with distanceOpacity

  • did not push npm pacakge.json. was just a npm init

  • default iconUrl points to cdn if leaflet comes from cdn so that will not work

  • in my original version some function had different name, but i kept them the same in pull request

    • _addEdgeMarker -> update
    • destroy -> remove
  • your version (including with pull request) delete an recreate in _addEdgeMarker,
    while i did update the existent marker and created them when not there.
    don't know if that makes a difference. but maybe something to concider

  • onclick not working on mobile for me

hopes this helps, you can tell me when you want me to change something.

Cheers

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