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(InfoWindowReactChild): allow React elements as content for InfoWindow and InfoBox (which can be styled completely, unlike InfoWindow) #69

Closed

Conversation

thetiby
Copy link

@thetiby thetiby commented Jun 16, 2015

@tomchentw implemented the API suggested by you in #25 (comment)

The usage would be:

  1. don't specify string content for the InfoWindow or InfoBox; instead, add a React child element to it
  2. use the wrapperClassName for the child element in order to give that class name to the wrapper div (for CSS styling, etc)

Important!

InfoBox won't allow any React events to bubble unless you give the enableEventPropagation={true} prop to it; see the whole API here: http://google-maps-utility-library-v3.googlecode.com/svn/trunk/infobox/docs/reference.html

The prop is disabled by default because it allows interaction with the map "through" the infobox when the above is set. Because React uses event delegation to trigger the handlers, couldn't find a way to cancel this effect. I'm considering using react-native-listener to solve this. Update: it works, though it's a bit hacky; I welcome suggestions

cc @metalmatze

@metalmatze
Copy link

Nice! Altough I can't say much about the code quality. ☺️

@@ -49,6 +49,10 @@ class ClosureListeners extends React.Component {
this.setState(this.state);
}

_handle_infowindow_click () {

Choose a reason for hiding this comment

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

Is something missing in this method? Or could it be removed aswell as the console.log?

Copy link
Author

Choose a reason for hiding this comment

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

@metalmatze this is just a test to see if it's working; all changes in this file can be removed; I know the code is rough, but I wrote this at the end of my work day today and was tired; I was hoping to get advice on improving this code, as it doesn't feel very robust; it does the job though - so I don't need to switch libs in my project; when it's a bit more refined, I'm sure it'll get in the main repo

@thetiby thetiby force-pushed the infowindow-react-component-content branch 13 times, most recently from e0b4ffb to f673970 Compare June 19, 2015 10:09
@thetiby
Copy link
Author

thetiby commented Jun 19, 2015

@tomchentw I updated my PR - it no longer uses hardcoded tests inside SimpleChildComponent - please let me know if you see any other improvement to be made before merging this new feature

@thetiby thetiby force-pushed the infowindow-react-component-content branch 2 times, most recently from 7730653 to 9241bd0 Compare June 19, 2015 15:35
@thetiby thetiby changed the title feat(InfoWindowReactChild): allow React elements as content for InfoWindow feat(InfoWindowReactChild): allow React elements as content for InfoWindow and InfoBox (which can be styled completely, unlike InfoWindow) Jun 19, 2015
@wuct
Copy link
Collaborator

wuct commented Jun 28, 2015

I like the new API, which moves content from props to children. Though, I think putting _handleMissingContentProp() in SimpleChildComponent is not a good idea, because only InfoBox and InfoContent use this method. @tomchentw what do you think?

@thetiby thetiby force-pushed the infowindow-react-component-content branch 2 times, most recently from 1bd29ba to 792f61f Compare July 22, 2015 15:34
@thetiby thetiby force-pushed the infowindow-react-component-content branch from 792f61f to 5314502 Compare July 22, 2015 15:40
@thetiby
Copy link
Author

thetiby commented Jul 22, 2015

@tomchentw hey there, can I improve anything else about this PR? I did several more updates to it; thanks for the great project!

@tomchentw
Copy link
Owner

TODO: add this in #88

Hi @thetiby @metalmatze ,

I took a day to rewrite the module from scratch on #88 . It will be great if you could take a look at it. Thanks!

tomchentw added a commit that referenced this pull request Aug 7, 2015
* Also add this support to InfoBox as well
* Only one child is supported inside <InfoWindow> or <InfoBox>
* Closes #69 and thanks to @thetiby
tomchentw added a commit that referenced this pull request Aug 7, 2015
* Also add this support to InfoBox as well
* Only one child is supported inside <InfoWindow> or <InfoBox>
* Closes #69 and thanks to @thetiby
tomchentw added a commit that referenced this pull request Aug 7, 2015
* Also add this support to InfoBox as well
* Only one child is supported inside <InfoWindow> or <InfoBox>
* Closes #69 and thanks to @thetiby
@tomchentw tomchentw closed this in 2c0dc02 Aug 7, 2015
@tomchentw
Copy link
Owner

Released v2.0.0 and v2.0.1.

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.

4 participants