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 feature with point is shown as a circle #209

Open
aloxe opened this issue Apr 29, 2024 · 10 comments
Open

A feature with point is shown as a circle #209

aloxe opened this issue Apr 29, 2024 · 10 comments

Comments

@aloxe
Copy link
Contributor

aloxe commented Apr 29, 2024

The feature in the example https://pigeon-maps.js.org/docs/geo-json

  features: [
    {
      type: "Feature",
      geometry: { type: "Point", coordinates: [2.0, 48.5] },
      properties: { prop0: "value0" },
    },
  ],

renders as

<svg width="667" height="300" viewBox="0 0 667 300" fill="none" xmlns="http://www.w3.org/2000/svg">
  <g clip-rule="evenodd" style="pointer-events: auto;">
    <circle cx="67.27860400017107" cy="178.1113652196019" fill="#d4e6ec99" stroke-width="1" stroke="white" r="20"></circle>
  </g>
</svg>

This may be useful for some cases but I would not use it as the default behaviour to display a point. When a trace is shown by joining points, this gives an ugly line of circles.

Screenshot 2024-04-29 at 8 02 14
@aloxe
Copy link
Contributor Author

aloxe commented Apr 29, 2024

@mariusandra I don't know what you had in mind in this case but I think that a point should not trace anything but a dot. This may be used with "properties" added to the feature but I would keep this outside of pigeon.

@aloxe
Copy link
Contributor Author

aloxe commented May 4, 2024

I tried the same trace with leaflet and there is also an issue with points because leaflet marks them with a marker.

Screenshot 2024-05-03 at 22 45 37

Nevertheless, I think this could be good if we could choose how we want the points to appear on the map.

@mariusandra
Copy link
Owner

Hey, seems like a bug. Any chance you'd be up for submitting a PR to fix it in some way? I'm not actively using this library myself, so never stumbled upon it, and need to find time to work on a fix. Happy to quickly merge a fix though.

@baldulin
Copy link
Contributor

baldulin commented Jun 5, 2024

  1. If you want to render a line segment change the geometry from type Point to LineString. Cause I'm not sure why you don't like leaflet either

  2. If you want to change the look of the points: You should be able to change the radius of the circle using styleCallback and some other features.

  3. If you want to pass a custom component, pass it into here using props. Requires a pr obviously:

    const renderer = {
    Point: PointComponent,
    MultiPoint,
    LineString,
    MultiLineString,
    Polygon,
    MultiPolygon,
    }

    And the correct one will be selected here:
    const Component = renderer[type]

@aloxe
Copy link
Contributor Author

aloxe commented Jun 6, 2024

Ahoj @baldulin I finally figured out this is not a bug. A Point feature can be represented in any manner on a map as it is a single point. A circle is not a bad idea even if I prefer the idea of a Marker.

  1. If you want to render a line segment change the geometry from type Point to LineString. Cause I'm not sure why you don't like leaflet either

If you mean that I should edit my geojson this is actually what I did. But about any anyone that simply want to use pigeon-map to display a kml file that contains many points like my example

  1. If you want to change the look of the points: You should be able to change the radius of the circle using styleCallback and some other features.

This is indeed the detail I took time to figure out, I think the doc could gain in saying that the r:30 is the radius of the circle.

  1. If you want to pass a custom component, pass it into here using props. Requires a pr obviously:
    const renderer = {
    Point: PointComponent,
    MultiPoint,
    LineString,
    MultiLineString,
    Polygon,
    MultiPolygon,
    }

Well this is not that trivial because the features don't accept the same props and a Point isn't a LineString. I am thinking about using the styleCallback to send an alternate svg to display instead of the circle. That could make it more flexible without making pigeon heavier.

@baldulin
Copy link
Contributor

baldulin commented Jun 7, 2024

This is indeed the detail I took time to figure out, I think the doc could gain in saying that the r:30 is the radius of the circle.

Hmm, yeah the documentation is very basic. I mean there is this example here doing exactly that. But maybe it needs some improvement.

Well this is not that trivial because the features don't accept the same props and a Point isn't a LineString. I am thinking about using the styleCallback to send an alternate svg to display instead of the circle. That could make it more flexible without making pigeon heavier.

I guess I'll just have a go at this then. Cause the style callback is a bit of a messy way of changing the styling, and I think that that pattern won't work for changing the component.

@aloxe
Copy link
Contributor Author

aloxe commented Jun 7, 2024

I guess I'll just have a go at this then. Cause the style callback is a bit of a messy way of changing the styling, and I think that that pattern won't work for changing the component.

You are right, you can't change the component since it is nested into the geoJson. What you can do is draw something else that a circle in some cases. This is what I did in the PR #211 . You may want to have a look and comment.

@baldulin
Copy link
Contributor

@aloxe

Okay yeah, obviously it's not my decision if that pr gets merged. But my opinion on it is that, firstly it works.
Now it is possible to render points as svg path elements whenever path is set in the styleCallback. This however makes that functionality even more messy and weird to document.
Secondly while this fixes your personal issue it does not address pretty similar issues others might have. Maybe they don't want a path or so on. Or they want to change the style of the LineSegment. So you could try to fix this issue in a way where it would help others later on too.

Rereading your earlier post I should have probably commented more on if it is trivial to exchange the full component. The diff below shows how that could be done.
My issues with that approach right now is that it's limiting what the component could render (must be an svg element). So I have to think about it a bit more.
But the pros are that it's just one prop and the way a custom marker would be implemented can be easily documented by the current point implementation. And would work for any GeoJson feature including custom ones.

p.s.
I noticed the comment about trying the changes patching index.esm.js, you can test out these things by changing the demo/Demo.tsx file like in the diff and run the project.

diff --git a/demo/Demo.tsx b/demo/Demo.tsx
index b76ef33..043d16f 100644
--- a/demo/Demo.tsx
+++ b/demo/Demo.tsx
@@ -257,8 +257,21 @@ export function Demo(): JSX.Element {
               }
               return { fill: '#d4e6ec99', strokeWidth: '1', stroke: 'white', r: '20' }
             }}
+            renderer={{
+              Point: ((props) => {
+                const [left, top] = props.latLngToPixel([props.coordinates[1], props.coordinates[0]])
+                return <g style={{ pointerEvents: 'auto' }} transform={`translate(${left} ${top})`}>
+                  <path
+                    d="M52 31.5C52 36.8395 49.18 42.314 45.0107 47.6094C40.8672 52.872 35.619 57.678 31.1763 61.6922C30.7916 62.0398 30.2084 62.0398 29.8237 61.6922C25.381 57.678 20.1328 52.872 15.9893 47.6094C11.82 42.314 9 36.8395 9 31.5C9 18.5709 18.6801 9 30.5 9C42.3199 9 52 18.5709 52 31.5Z"
+                    fill="red"
+                    stroke="white"
+                    strokeWidth="4"
+                  />
+                  <circle cx="30.5" cy="30.5" r="8.5" fill="white" opacity={0.98} />
+                </g>
+              }) as React.FC,
+            }}
           />
          <ZoomControl />
         </Map>
       </div>
       <div>
diff --git a/src/overlays/GeoJson.tsx b/src/overlays/GeoJson.tsx
index a35d315..6e3da13 100644
--- a/src/overlays/GeoJson.tsx
+++ b/src/overlays/GeoJson.tsx
@@ -10,6 +10,7 @@ interface GeoJsonProps extends PigeonProps {
   feature?: any
   style?: CSSProperties
   children?: React.ReactNode
+  renderer?: Map<string, React.ElementType>

   // callbacks
   onClick?: ({ event: HTMLMouseEvent, anchor: Point, payload: any }) => void
@@ -41,6 +42,7 @@ interface GeometryProps {
   latLngToPixel?: (latLng: Point, center?: Point, zoom?: number) => Point
   svgAttributes?: SVGProps<SVGElement>
   geometry?: GeoJsonGeometry
+  renderer?: Map<string, React.ElementType>
 }

 const defaultSvgAttributes = { fill: '#93c0d099', strokeWidth: '2', stroke: 'white', r: '30' }
@@ -119,6 +121,7 @@ export function GeometryCollection(props: GeometryProps): JSX.Element {
     MultiLineString,
     Polygon,
     MultiPolygon,
+    ...props.renderer
   }

   const { type, coordinates, geometries } = props.geometry

@alix-guillard-deltatre
Copy link

@baldulin, My PR got merged, but it didn't close this issue. This is good because I wanted to discuss your last comment.

Two things, the issue I reported is in fact not a bug and I just found out how to avoid my problem, just my removing Points from my geojson tracks.

Nevertheless, I thought this could be a nice option to display points as a custom path rather than a circle. I don't think users would want to change the style of other GeoJson elements are quite meaningful as they are.

Thanks for the code, this is a nice way to test pigeon-maps without having to install it in another project. Unfortunately, I can't make it work as is.

I also have a question, since it looks like you want to display the circle at the same time as the path. Do you think users would wish that?

Also I see you added the tags around the path in your renderer. This could be nice to use an SVG group for the Point. I felt this wasn't possible given how is designed GeoJson component, but do you think this could be feasible?

@baldulin
Copy link
Contributor

baldulin commented Jun 24, 2024

@alix-guillard-deltatre

My PR got merged

Nice, Congratz!

Nevertheless, I thought this could be a nice option to display points as a custom path rather than a circle. I don't think users would want to change the style of other GeoJson elements are quite meaningful as they are.

Yeah like I said, it work's and that option should be there. Like you say it's just the point that can have arbitrary representations (I always thought that nobody would really need that point, because either you use a marker or some kind of clustering). The reason to extend all other elements, is not really because they need to. But just to have a generalized way of modifying components. This obviously has a lot of benefits (like Lifecycle stuff, reusability and so on). But it's also easier to document.

Also I see you added the tags around the path in your renderer. This could be nice to use an SVG group for the Point. I felt this wasn't possible given how is designed GeoJson component, but do you think this could be feasible?

And yeah like you say you can use svg groups as well. You could add text or anything you want (but you are in the svg context already). Because the renderer here takes a component, it does not inject props into an existing component. It should already work. If you couldn't run it, than its not apparent, but that code basically renders the default pigeon marker in red, I just copied it.

I also have a question, since it looks like you want to display the circle at the same time as the path. Do you think users would wish that?

I don't know what users wish to be quite honest. Like I said I used both because the default pigeon marker uses both. It's just again a repetition of "if a users wishes to change the look of a point they should supply their own component instead of making a prebuild component look like they want to."

Thanks for the code, this is a nice way to test pigeon-maps without having to install it in another project. Unfortunately, I can't make it work as is.

Yeah I guess you having issues with ERR_OSSL_EVP_UNSUPPORTED. This project needs to be updated quite a lot. Should work with export NODE_OPTIONS=--openssl-legacy-provider though.

In the end you have to ask yourself if your fine with it as it is right now. As long as you are happy this issue is fixed. I got more hesitant to fix anything here because I thought it more prudent to upgrade the project.

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

No branches or pull requests

4 participants