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

feat: created 'Circle' geometry component (#105) #178

Merged
merged 7 commits into from
Jan 31, 2024

Conversation

ArielBenichou
Copy link
Contributor

@ArielBenichou ArielBenichou commented Jan 24, 2024

I have implemented a new React component for rendering circles on Google Maps within the existing library. Drawing inspiration from the Marker component, I made some code refinements, such as relocating the callbacks (e.g., onClick, onDrag) to a ref. This adjustment ensures that the useEffect does not need to be re-executed unnecessarily, addressing potential issues with un-memoized functions passed by the parent component. I encountered this issue involving an infinite feedback loop caused by this scenario.

Furthermore, I've included a sample application under geometry that showcases the circle component. This app allows users to interact with and manipulate the circle either through the map or via input controls.

I've also based the doc on the Marker markdown file.

This is my first time PR to an open-source project, so if i missed something, the feedback is welcome, thanks

Changes Made

  • Added a new React component for circles on Google Maps.
  • Introduced a sample application (geometry) demonstrating circle manipulation via map interactions and input controls.
  • Created documentation in the docs/circle.md file for easy reference.

@usefulthink
Copy link
Collaborator

First of all: WOW! Thanks so much for this. I hope I can make time later today to review, but from a quick glance looks amazing 🤩

@ArielBenichou
Copy link
Contributor Author

Initial Values

should we add an initialRadius and initialCenter to the <Circle /> ?
imagine a case where you want to draw a Circle that is editable/draggable but not controlled via a state.

const [center, setCenter] = React.useState({lat: 41.23296, lng: -95.877363});
const [radius, setRadius] = React.useState(15000);

return (
  <>
    <Circle
        radius={radius}
        center={center}
        onRadiusChange={setRadius}
        onCenterChange={setCenter}
        draggable
        editable
     />
    <Circle 
        radius={10000}
        center={{lat: 41, lng: -95}}
        draggable
        editable
     />
  </>
)

in the case above, because we have one controlled component and one that is uncontrolled
the controlled component will trigger a re-render and reset the value of the uncontrolled component - snapping it back to it original position
using initials values for this will solve our problem.

@ArielBenichou
Copy link
Contributor Author

ArielBenichou commented Jan 25, 2024

After some fixes for the <Circle />,
I've also implemented the <Polygon /> component, giving me this in total:
image

I will wait now for feedback.
Thanks for the collaboration.

@usefulthink
Copy link
Collaborator

This is all very excellent stuff. I'm impressed by the overall quality of your work! I will definitely have a closer look at some point and adapt some of the patterns you used in the tests. I will add some notes to the code for some smaller issues I spotted.

The only thing I would like to see changed for now is to move all additions you made to the main source tree into the example folder. So ./src/components would move to ./examples/geometry/src/components, and ./docs/{circle,polygon}.md would be moved to ./examples/geometry/docs/.

The lat/lng library functions can stay, but they could go together with the existing is-lat-lng-literal.js and become lat-lng-utils.ts or something (maybe also split everything, but I'm not a big fan of such small files...).

The reasoning for this is that we want to take additions to the main library a bit slower, and give new features some time to mature as part of the examples. This also gives us some time to think about if and how exactly we want to add them to the library (I wrote a bite more about this here).

In the end I want to prevent having to make changes to features after they are merged and published with the library, and thus trigger a major release. On the other hand, I don't see a problem with updating the code of an example if we discover a way to do things differently.

@ArielBenichou
Copy link
Contributor Author

I've shuffled things around as you suggested - components and docs are now part of the "geometry" example.

I went ahead and bundled up the lat-lng utilities into one file (lat-lng-utils.ts). I'm on the same page of not wanting to split every function into its own thing.

About the tests, I scrapped them. They were pretty much a copy-pasta job from the marker tests with a few tweaks. Nothing too fancy.

And yeah, I'm totally get your approach of taking it slow before shoving stuff into the main package. Letting things marinate a bit sounds like a smart move.

Let's me know about anything else.

Cheers

Copy link
Collaborator

@usefulthink usefulthink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thanks. A couple more small notes, then we can get this merged.

examples/geometry/README.md Outdated Show resolved Hide resolved
docs/api-reference/components/polygon.md Outdated Show resolved Hide resolved
examples/geometry/index.html Outdated Show resolved Hide resolved
examples/geometry/README.md Show resolved Hide resolved
examples/geometry/src/control-panel.tsx Outdated Show resolved Hide resolved
examples/geometry/src/control-panel.tsx Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file, without any comment kinda scares me. Should at least state what data this is, why it looks like this and how it was generated.

Maybe can be renamed to encoded-polygon-data.ts or something like that to make this more clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah that a big scary file, TBH i took this from the project i'm working on, maybe it is better to simplify it to a simple triangle?
I just thought it looked cool XD

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, thats perfectly fine, examples should be looking interesting and useful :)

I think it just needs a comment that those are polygons of the districts of Omaha, Nebraska (?) in the encoded polyline algorithm format, with a link to this: https://developers.google.com/maps/documentation/utilities/polylinealgorithm and maybe a link to some tool you used to generate them.

examples/geometry/src/components/circle.tsx Show resolved Hide resolved
examples/geometry/src/components/circle.tsx Outdated Show resolved Hide resolved
examples/geometry/src/components/polygon.tsx Outdated Show resolved Hide resolved
ArielBenichou and others added 7 commits January 31, 2024 18:51
to mitigate an inifinite re-render when the <Circle /> is being controlled,
I've splitted the radius, and center from the other events, because they are listened to.
I've added a equality guard before calling the setCenter and setRadius
I was using useMemo (sync) instead of useEffect (async) to update the google.maps.circle instance
when the props where changing, causing setState to the parent when the child was rendering
can recieve `encodedPaths` instead of `paths`
merged docs to the example README
merged both contorl panel to be in the same one
added comments and docs
@usefulthink usefulthink merged commit 31f2655 into visgl:main Jan 31, 2024
2 checks passed
@usefulthink
Copy link
Collaborator

Again, thanks a lot. Last thing to do is to update the website files to add the example to the overview page, do you want to do that? The steps for that are described here: https://visgl.github.io/react-google-maps/docs/guides/writing-examples#adding-examples-to-the-website

@ArielBenichou
Copy link
Contributor Author

No problem, let's do things all the way.
I will create a new PR for the website example.

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