-
-
Notifications
You must be signed in to change notification settings - Fork 299
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
[Map] Adding polygons to google and leaflet + info window #2162
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.
Thank you for working on this @rrr63!
It's not far from perfect, but I do have a few comments.
Also, don't forget to update Map
's CHANGELOG.md files, thanks :)
const polygon = { polygon: 'polygon', title: definition.title }; | ||
|
||
if (definition.infoWindow) { | ||
this.createInfoWindowPolygon({ definition: definition.infoWindow, polygon }); |
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.createInfoWindowPolygon({ definition: definition.infoWindow, polygon }); | |
this.createInfoWindow({ definition: definition.infoWindow, polygon }); |
@Kocal Thank for your comments, i fixed it. |
However, I wonder because a test did not pass on the Autocomplete component by just changing the Map's changelog ... |
Weird, it works by adding the changelog : |
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.
Minor comments, and I think it will be good for me!
I will try to find some time to try what you did :)
const latLngs = points.map((point) => ({ lat: point.lat, lng: point.lng })); | ||
|
||
const polygon = new _google.maps.Polygon({ | ||
...rawOptions, | ||
paths: latLngs, |
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.
points
is already a list of { lat: number; lng: number }
, so it should be enough no?
const latLngs = points.map((point) => ({ lat: point.lat, lng: point.lng })); | |
const polygon = new _google.maps.Polygon({ | |
...rawOptions, | |
paths: latLngs, | |
const polygon = new _google.maps.Polygon({ | |
...rawOptions, | |
paths: points, |
I've just merged #2117, you will need to rebase your PR and add Feel free to ask help if needed |
Hello @Kocal Thanks |
Locally after a rebase on 2.x You need to rebase / fix the conflcits / force-push here :) Now it's a bit hard to review 90 files 😅 |
I'll add then a couple of comments, regarding argument order and default values :) |
Oh so sorry, Do you think it will be easier for you if I create a new PR? |
I will see what I can do |
I like it very much ! Would you agree, afterwards, to help me prepare a demo using polygons on the website ? |
Yes I could, with pleasure :) |
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.
Final reviews 🙏🏻
protected abstract doCreateInfoWindow({ | ||
definition, | ||
marker, | ||
element, | ||
}: { | ||
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow']; | ||
marker: Marker; | ||
definition: | ||
| MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'] | ||
| PolygonDefinition<PolygonOptions, InfoWindowOptions>['infoWindow']; | ||
element: Marker | Polygon; | ||
}): InfoWindow; |
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 be improved to:
protected abstract doCreateInfoWindow({
definition,
element,
}: {
definition: MarkerDefinition<MarkerOptions, InfoWindowOptions>['infoWindow'];
element: Marker;
} | {
definition: PolygonDefinition<PolygonOptions, InfoWindowOptions>['infoWindow'];
element: Polygon;
}): InfoWindow;
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.
Ok, thanks for this review !
anchor: marker, | ||
if (element instanceof google.maps.marker.AdvancedMarkerElement) { | ||
element.addListener('click', () => { | ||
if (definition.autoClose) this.closeInfoWindowsExcept(infoWindow); |
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.
Not a fan of one-lined condition + code, as it can easily introduce mistakes, please prefer keeping this form:
if (definition.autoClose) this.closeInfoWindowsExcept(infoWindow); | |
if (definition.autoClose) { | |
this.closeInfoWindowsExcept(infoWindow); | |
} |
Note: I will see if Biome can prevent this
anchor: marker, | ||
} else if (element instanceof google.maps.Polygon) { | ||
element.addListener('click', (event: any) => { | ||
if (definition.autoClose) this.closeInfoWindowsExcept(infoWindow); |
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.
Not a fan of one-lined condition + code, as it can easily introduce mistakes, please prefer keeping this form:
if (definition.autoClose) this.closeInfoWindowsExcept(infoWindow); | |
if (definition.autoClose) { | |
this.closeInfoWindowsExcept(infoWindow); | |
} |
Note: I will see if Biome can prevent this
I'm taking care of the rest, rebasing and fixing conflicts, testing the feature on my side, and merge :) |
It took time, but here we go, this is in now. Thank you very much @rrr63. |
Hello,
In my job i use to work a lot with maps, so i would love to work with symfony ux in next projects.
But i have to work with polygons, so i tried to implement it to the ux-map.
I added polygons on the Map component, and made it work on both bridge : google and leaflet.
I added a window popup on click also, the current version of window popup worked only with markers.
I had to rename
createInfoWindow
tocreateInfoWindowMarker
. With that i could add acreateInfoWindowPolygon
.The only difference with the infoWindow from marker is that his anchor will be the event cursor position, because the area of the polygon could be very large and we want a window on the click position.
You can try by adding a simple polygon on your map :
Here a full example that works with leaflet and google that show some of french regions via an api call
I added tests for polygons and polygon info window
I hope I did it right, i listen for any suggestions or edits in the PR