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

turf-point-to-polygon-distance package #1743 #2735

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

Conversation

pachacamac
Copy link

@pachacamac pachacamac commented Oct 25, 2024

Please fill in this template. Use a meaningful title for the pull request. Include the name of the package modified.

  • Is this a bug fix, new functionality, or a breaking change?
    • new functionality
  • Have read and followed the steps for preparing a pull request.

Submitting a new TurfJS Module.

  • Overview description of proposed module.
  • Include JSDocs with a basic example.
  • Execute ./scripts/generate-readmes to create README.md.
  • Add yourself to contributors in package.json using "Full Name <@github Username>".

@smallsaucepan
Copy link
Member

smallsaucepan commented Oct 26, 2024

Looking good thanks @pachacamac. Glad to see it works with polygon holes as well.

Before it's ready to merge we might need to flesh it out in a couple of areas:

  1. Allowing options in line with sister functions like pointToLineDistance. For example, being able to pass Units (meters, miles, yards).
  2. Accepting the widest possible "complexity" of meaningful arguments. So, for the point a user can pass a simple number array (Position), or a bare geometry (Point), or a full on feature with properties (Feature<Point>). So the arguments would look like this instead:
    point: Feature<Point> | Point | Position
    polygon: Feature<Polygon | MultiPolygon> | Polygon | MultiPolygon

Sorry if that's more involved than you expected. Happy to co-author though if your time is limited.

@pachacamac
Copy link
Author

Thanks for pointing out the options. Missed that I can just forward this to the pointToLineDistance function. I also added a test that roughly reflects the initial issue question. For the sake of not complicating my test fixture setup I won't print out which point is the closest but pointB is the closest ;) which can be checked by comparing the expected distances of the three fixture points in the long-lines-poly.geojson.
I also added the expected unit to the test fixtures to validate that the unit conversion is working as expected.

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