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

Added MarkerWithLabel component (based on markerwithlabel). #917

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

BowlingX
Copy link

@BowlingX BowlingX commented Jun 4, 2020

This PR adds the MarkerWithLabel component that allows you to create a custom marker from a react-component. Fixes #782, fixes #219

I'm unable to run the linter locally as it seems like prettier is missing in the devDependencies, or it is suppose to be done differently?

I decided to extend the current Marker component instead of extending it's current functionality to give the user the choice to include the markerwithlabel dependency or not. I think in this case inheritance makes sense.

Thank you

@JustFly1984
Copy link
Owner

@BowlingX Sorry, but this library does not use external dependencies, especially which does not support typescript.. You can fork markerwithlabel, and place it in packages/react-google-maps-api-marker-with-label and reuse tsdx setup from other packages to refactor it to typescript, and after that I will publish it and include as dependency to main project. Please continue in same PR.

@BowlingX
Copy link
Author

BowlingX commented Jun 4, 2020

Thank you for having a look :). Rewriting the library / Adding types for it is out of my scope currently timewise, but I can provide types for it, as it basically just extends google.maps.Marker and takes some more properties (extending google.maps.MarkerOptions). Let me know if this can work as well.

@BowlingX
Copy link
Author

BowlingX commented Jun 4, 2020

I also tried to use the current version directly from google (@google/markerwithlabel), but it seems there is a bug on the latest version (see googlemaps/v3-utility-library#651). I ran into the same issue.

@JustFly1984
Copy link
Owner

@BowlingX well, if you clone markerclusterer to packages/react-google-maps-api-marker-with-label, I will help to refactor it to typescript and publish

@JustFly1984 JustFly1984 changed the base branch from develop to v2.0.0-alpha June 7, 2020 17:21
@JustFly1984
Copy link
Owner

@BowlingX I've spent a weekend to give some love to the project. I had rebased your PR to 2.0.0 version branch. I have created new package marker-with-label, but had no time to refactor it to TS yet. Will continue then I will have time again. BTW I've published 2.0.0-alpha and would love if you test it.

@JustFly1984
Copy link
Owner

PS please fix merge conflicts

@BowlingX
Copy link
Author

BowlingX commented Jun 8, 2020

I can't test the implementation currently as it seems like there are several issues:

VM762:11 Uncaught SyntaxError: Identifier 'InfoBox' has already been declared
    at new Function (<anonymous>)

...and something is triggering and endless loop:

Warning: Maximum update depth exceeded. This can happen when a component calls setState inside useEffect, but useEffect either doesn't have a dependency array, or one of the dependencies changes on every render.
    in ScriptLoaded (created by bound anonymous)
    in bound anonymous (created by ReactExample)
    in Wrapper (created by ReactExample)
    in ReactExample

@@ -33,7 +34,13 @@ const markerWithLabelUpdaterMap = {
...updaterMap,
}

const MarkerWithLabel: FC<MarkerWithLabelProps> = props => {
const MarkerWithLabel: FC<MarkerWithLabelProps> = ({
labelAnchor,
Copy link
Author

Choose a reason for hiding this comment

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

Something also seems to be wrong with the eslint setup, I get warnings where I should not get them.

Copy link
Owner

Choose a reason for hiding this comment

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

can you attach a screenshot of warnings?

I will continue to work on alpha version this week, tried alpha in codesandbox example and it fails.

@BowlingX
Copy link
Author

BowlingX commented Jun 9, 2020

I have a general question, why not using the new @google packages for the implementations (e.g. @google/markerclustererplus etc.) instead of porting /copying the dependencies. It seems redundant to me and will make maintainability harder espacially when those are maintained by google directly against there map API.

@JustFly1984
Copy link
Owner

@BowlingX if it does not support types, it is buggy. It is still buggy as it is. It is not possible to be involved in maintenance of google packages. General quality is sucks. I'm trying to improve general dev experience, and use it as learning experience for myself.

@JustFly1984 JustFly1984 changed the base branch from v2.0.0-alpha to develop June 15, 2020 09:28
@JustFly1984 JustFly1984 changed the base branch from develop to v2.0.0-alpha June 15, 2020 09:29
@JustFly1984 JustFly1984 changed the base branch from v2.0.0-alpha to develop July 18, 2020 14:00
@sanath-sfs
Copy link

Thanks guys. any idea when this will get merged?

@JustFly1984
Copy link
Owner

at this point this PR requires cherrypick to another PR to development branch. Sorry I just have no time to do that. If somebody want to be a champion, I will review and merge, and release new version.

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.

Integrate markerwithlabel to extend Marker styling options Addition of MarkerWithLabel component
3 participants