Skip to content

Improve the example code #304

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

Closed
wants to merge 2 commits into from
Closed

Improve the example code #304

wants to merge 2 commits into from

Conversation

aaronmader
Copy link

Update the example code to include a marker definition (MyGreatPlace) for completeness.
Addresses issue #300

Update the example code to include a marker definition (MyGreatPlace) for completeness.
@istarkov
Copy link
Collaborator

istarkov commented Jan 16, 2017

Thank you! But -

This is readme example.

  • It must be short.
  • Just before example I wrote you just need to add lat lng props to any child of GoogleMap component
  • At first sentences of readme I wrote It allows you to render any React component on the Google Map
  • import contains ./ which means that it's some your component
  • there are a lot of working examples refs in readme even on jsbin.

So sorry I don't think this is needed.

@aaronmader
Copy link
Author

Solid points.. I did eventually find my way over to https://github.com/istarkov/google-map-react-examples which was a HUGE help.

I got stuck because I was assuming that there was more to the MyGreatPlace component. (And I suppose that's where the other +1's in the issue were struggling too). You're instruction you just need to add lat lng props to any child of GoogleMap component makes sense to me now, but at the time I was just trying to copy/paste your example into my react project, without success.

Maybe if we just add another simple example using a div? (I'll create a pull request to show what I mean)

Add a simple marker example for first-time users.
@Sapphire64
Copy link

I was thinking about improving that part too and suddenly found this PR already in place :) MyGreatPlace has to be explained, for me passing of geo attributes to every kinds of components seems to be unclear (but I agree, this is useful), so I had to dig into examples too. I thought you need to handle the coordinates inside of the component.

So I am +1 on current patch or maybe can suggest to apply following patch to have only one usage of "MyGreatPlace" in the example:

- <MyGreatPlace lat={59.955413} lng={30.337844} text={'A'} /* Kreyser Avrora */ />
+ <div lat={59.955413} lng={30.337844}>{'A'}</div> /* Kreyser Avrora */

MyGreatPlace is also vague to be fair, if we change it to something in the context like RedMarker it will explain more that it's a wrapper, not the actual target object with coordinates like you can misunderstand now.

In general I would prefer to have next interface when it comes to markers on the map to make it less confusing:

<Marker key={} lat={} lng={}>
    <YourComponentHere/>
</Marker>

But that's another story.

@hjmccain
Copy link

The lack of detail about MyGreatPlace was a source of frustration for me, as well, and led me to initially pass up this module. @istarkov your argument is that the Readme needs to be short — explaining MyGreatPlace module would barely lengthen it at all — four sentences, maybe, and a line or two of code? Worth it for the clarity!

@istarkov
Copy link
Collaborator

Thank you all, please look at current readme
#320

@aaronmader aaronmader deleted the aaronmader-patch-1 branch February 16, 2017 19:27
@lock
Copy link

lock bot commented Dec 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants