Update SimpleMap Component Example in README.md #251
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I may be missing something, but from what I could tell, the SimpleApp code example listed in the README file is not actually valid code... I was unable to get it working without bind() errors. It also heavily uses the 'this' keyword and .bind() despite being defined as a pure (stateless) functional component, which is uncommon and a little confusing (especially if someone were to define it using an arrow function without understanding how it differs). I updated it to be more like the sample SimpleMap linked here: https://github.com/tomchentw/react-google-maps_examples_simple-map/blob/493c95b5830a36434d648b9d7f779f231d2c79a5/src/scripts/SimpleMap.js For now, I replaced the {...props} spread in GoogleMapLoader with {...props.containerElementProps}, as the click handlers are now being passed in as props too. I'm not sure if this is the best approach though... If you don't like this solution (where everything needed is passed in as a prop), maybe the example should use an ES6 class instead of a pure function?
|
Thanks! The changes are great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I may be missing something, but from what I could tell, the SimpleMap code example listed in the README file is not actually valid code... I was unable to get it working without bind() errors. It also heavily uses the 'this' keyword and .bind() despite being defined as a pure (stateless) functional component, which is uncommon and a little confusing (especially if someone were to define it using an arrow function without understanding how it differs).
I updated it to be more like the sample SimpleMap linked here:
https://github.com/tomchentw/react-google-maps_examples_simple-map/blob/493c95b5830a36434d648b9d7f779f231d2c79a5/src/scripts/SimpleMap.js
For now, I replaced the {...props} spread in GoogleMapLoader with {...props.containerElementProps}, as the click handlers are now being passed in as props too. I'm not sure if this is the best approach though...
If you don't like this solution (where everything needed is passed in as a prop), maybe the example should use an ES6 class instead of a pure function?
EDIT: I think I may have overlooked something and that's why I was having issues with the .bind() calls. Sorry for that. However, I do think getting rid of 'this' and 'bind()' would be ideal when using a functional component as an example. Also, we could go one step further and use destructuring assignment in the SimpleMap parameters to get rid of the props references.