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

chore(src/components): Add MarkerWithLabel Component #657

Closed
wants to merge 0 commits into from

Conversation

d46
Copy link
Contributor

@d46 d46 commented Oct 9, 2017

I implemented MarkerWithLabel like a new component as @tomchentw said but i get some problems.

MarkerWithLabel wraps the google's Marker object so i decided to fetch prototypes from google document like in Marker macro. In this case i had to use Marker for class name to fetch Marker api.

Naming for Marker made errors like this

Encountered two children with the same key, `Marker`.

I didn't touch jscodeshit side but in this implementation maybe we need to change fetching data with class name scenario.

In the constructor i had to use ReactDOM.render. MarkerWithLabel require string for labelContent.
Also when we send null or empty string to labelContent it cause crash.

I used this in componentDidUpdate shallow compare too. What do you think for this approach?

Waiting your feedback to better implementation.

Copy link
Owner

@tomchentw tomchentw left a comment

Choose a reason for hiding this comment

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

Couple of changes need to be done:

  • Revert changes under lib and src/components. They should be auto-generated.
  • Fix review comments

Thanks!

.editorconfig Outdated
@@ -10,7 +10,7 @@ insert_final_newline = true

# Matches multiple files with brace expansion notation
# Set default charset
[*.{js,py}]
[*.{js,py,jsx}]
Copy link
Owner

Choose a reason for hiding this comment

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

Thank for this!

@@ -76,7 +76,7 @@ var Marker = (exports.Marker = (function(_React$PureComponent) {
)
)

var GMKlass = _this.props.markerWithLabel || google.maps.Marker
var GMKlass = google.maps.Marker
Copy link
Owner

Choose a reason for hiding this comment

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

Don't commit lib……yet

@@ -318,7 +288,7 @@ export class Marker extends React.PureComponent {
*/
constructor(props, context) {
super(props, context)
const GMKlass = this.props.markerWithLabel || google.maps.Marker
const GMKlass = google.maps.Marker
Copy link
Owner

Choose a reason for hiding this comment

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

Don't commit src/components yet. Should just src/macros

@@ -77,7 +52,7 @@ export class Marker extends React.PureComponent {
*/
constructor(props, context) {
super(props, context)
const GMKlass = this.props.markerWithLabel || google.maps.Marker
const GMKlass = google.maps.Marker
Copy link
Owner

Choose a reason for hiding this comment

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

Just inline this

/* global google */
import React from "react"
import PropTypes from "prop-types"
import _MarkerWithLabel from "markerwithlabel"
Copy link
Owner

Choose a reason for hiding this comment

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

Call it NativeMarkerWithLabel

super(props, context)
const markerWithLabel = new _MarkerWithLabel(google.maps)
const container = document.createElement("div")
ReactDOM.render(this.props.children, container)
Copy link
Owner

Choose a reason for hiding this comment

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

Don't render in the constructor method. Please see the corresponding code in InfoBox/InfoWindow

@tomchentw
Copy link
Owner

I'll look into Naming for Marker issue when you fixed the issues, thanks!

@d46 d46 force-pushed the master branch 5 times, most recently from ab4f28f to 0f66c25 Compare October 12, 2017 09:10
@tomchentw tomchentw closed this Oct 16, 2017
tomchentw pushed a commit that referenced this pull request Oct 16, 2017
@tomchentw
Copy link
Owner

@d46 I'm so sorry. Apologies for screwing up your repository and your master branch. The PR has been reworked to #669.

tomchentw pushed a commit that referenced this pull request Oct 16, 2017
@tomchentw
Copy link
Owner

Released v9.0.0

@abhayjeetgupta
Copy link

@tomchentw Thanks for the MarkerwithLabel feature But I am not able to add infowindow with the marker.

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.

3 participants