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

Contributors are welcome! #18

Open
12 of 17 tasks
uriklar opened this issue Dec 14, 2018 · 48 comments
Open
12 of 17 tasks

Contributors are welcome! #18

uriklar opened this issue Dec 14, 2018 · 48 comments
Labels
help wanted Extra attention is needed

Comments

@uriklar
Copy link
Collaborator

uriklar commented Dec 14, 2018

Hi everyone,
We'd love to get your code contributions! We have a slack channel that is currently on an invite basis. Leave a note here if you want to join.

There are some components that need to be revised from their old (react-google-maps) implementation, to our new implementation. For an examples of such a conversion, see this commit

Here are the components that still need to be implemented

  • StreetViewPanorama
  • StreetViewService
  • AutoComplete
  • StandaloneSearchBox
  • InfoBox
  • ImageMapType
  • Data
  • MarkerClusterer

Other open tasks:

  • Docs! You can see example mdx files in the docs folder. We want to achieve coverage for all components in the docs
  • Examples Gatsby.js
  • Examplex Create React App
  • Code cleanup (remove unused code leftover from react-google-maps)
  • Typescript support
  • Jest tests,
  • End-to-end tests
  • Write a migration guide from react-google-maps to react-google-maps-api
  • Publish docs as github pages (published on netlify)
@uriklar uriklar added the help wanted Extra attention is needed label Dec 14, 2018
@shafqatalix
Copy link
Contributor

Hi @uriklar , I am working on another fork of react-google-maps, but since you guys already started this one, i would like to contribute instead of perusing my own fork. I am working on project which uses react-google-maps, and i faced some issues for which i had to override some behaviors in some of the components.

  • OverlayView
  • Workaound (fix) for Google Maps Memory Leak mentioned in here
    I have Higher-Order-Component to get around this issues.
  • Few more addOns to help customizing maps UI.

Please let me know If that is something you would like to incorporate into this fork? Then i can start working on preparing PR. Plus one more thing this lib doesn't build on windows machine, and i have to add few things to package.json make is work.

@uriklar
Copy link
Collaborator Author

uriklar commented Dec 15, 2018

Hi @xalisys, we'd be happy to add you code. I saw your fork, but didn't see the commits you mentioned. Can you send me a link?

And a PR to fix the windows build would be greatly appreciated!

@shafqatalix
Copy link
Contributor

@uriklar thanks for quick response, the code i mentioned is in my project where i override the react-google-maps, components. I'll create separate PRs for things aforementioned.

thanks.

@gregoryfm
Copy link
Contributor

Hi @uriklar
I'm willing to help.
Can you add me in the slack group to let me know where to start?

@uriklar
Copy link
Collaborator Author

uriklar commented Dec 17, 2018

Hi @gregoryfm! Send me the email of your slack account and i'll send you an invite. (You can also mail me at uriklar@gmail.com if you prefer)

@gregoryfm
Copy link
Contributor

I sent you an email

@danielkcz
Copy link
Contributor

As I already mentioned somewhere else ... I would like to contribute, however seeing this project is almost dead already, I don't feel like it's worth the time. Did everyone get so busy suddenly to let this die?

@uriklar
Copy link
Collaborator Author

uriklar commented Jan 31, 2019

Hi @FredyC, Why do you say it's dead?
I think the lib has reached an MVP state and from now on it will grow to wherever people will have the need and the time to make it grow.

As Alexey mentioned in several places, we use this lib for our personal and company's projects, so our work on it is focused on those needs. But we will gladly help anyone who wants to contribute and will surely put in some more work in when we have the time for it.

@danielkcz
Copy link
Contributor

I totally understand it's not like the main project you want to invest too much time into. In that case, it might been better to just keep it private instead of saying everywhere that this is a replacement lib. How it can be when it's lacking features that react-google-maps have?

Do you even adhere to semver? With your "hobby" approach I feel like the stability could be rather volatile. There are no tests which I would say is kinda a must for a library. Otherwise, it's just a code that was mangled together to make some basic scenarios working (with fingers crossed). Documentation is rather sparse and it's hard to tell what's actually working and what isn't.

I do appreciate your valiant effort for sure, but at this point, I feel like I would just fork react-google-maps, cherry-pick commits from existing PRs and it would be much safer and easier for existing production apps.

@danielkcz danielkcz mentioned this issue Jan 31, 2019
@uriklar
Copy link
Collaborator Author

uriklar commented Jan 31, 2019

First of all, for the best of my knowledge react-google-maps doesn't have any tests either.
This is a project at the beginning of it's path. We made it public so we can start gathering react-google-map users and show them there's a home for their map needs if they wish to come and help build it.

I know that some components are missing, but, did you look at the code? It's so easy to just pick a component up and implement it! For documentation we use docz which makes adding docs even easier. I agree with you this project deserves more work then it gets..

If react-google-maps works for you, I don't see any reason to migrate. For me, the main use case for migrating from would have to be Async React readiness (which @react-google-maps/api is), also reduced bundle size is a nice touch.

@danielkcz
Copy link
Contributor

danielkcz commented Jan 31, 2019

First of all, for the best of my knowledge react-google-maps doesn't have any tests either.

Fair enough, there are a few basic tests, but probably not that useful.

I know that some components are missing, but, did you look at the code?

I did not. Honestly, seeing JS code base just gives me shivers as it means that touching anything can break stuff fairly easily (especially with no tests). Unless the decision for TypeScript happens, I don't think I will be able to contribute.

If react-google-maps works for you, I don't see any reason to migrate. For me, the main use case for migrating from would have to be Async React readiness (which @react-google-maps/api is), also reduced bundle size is a nice touch.

It works, but there are some quicks for sure that could be solved here better. For starters we have fully embraced React Hooks, so seeing class-based context is just bad :)

@uriklar
Copy link
Collaborator Author

uriklar commented Jan 31, 2019

We haven't used hooks in this project as they haven't been released yet. Once they're released this whole lib might as well be shrunk into a single hook :-)
You can see a (non-working) POC here: https://github.com/JustFly1984/react-google-maps-api/blob/master/packages/react-google-maps-api/src/utils/use-map-component.js

If you feel like making a PR to kick off the Typescript migration effort I will gladly join in on that branch!

@JustFly1984 JustFly1984 pinned this issue Feb 28, 2019
@JustFly1984
Copy link
Owner

@FredyC We have done refactor to typescript, if you have time can you please review?

@danielkcz
Copy link
Contributor

@JustFly1984 It's definitely on my todo list. Lately, more and more issues are coming from the abandoned maps project, so there is a hoping it will improve with this one.

@JustFly1984
Copy link
Owner

@FredyC We have published @1.2.0 with full typescript support.

@danielkcz
Copy link
Contributor

danielkcz commented Apr 10, 2019

Thank you for all the hard work. Tried to integrate into a project today and it was surprisingly easy to migrate. Not that it's some heavy map usage app, mostly just a couple of OverlayView, but 😍

I got a few pointers.

  • Export component props interfaces, it's super helpful when wrapping into other component and spreading props
  • Export MapContext or even better a useGoogleMap hook for accessing that context ... the onLoad is awkward for any serious work
  • Got a bit surprised by LoadScript requiring language and region, wasn't using those before and it worked, I assume it identifies language by Accept-Language header which should be sufficient in most cases.
  • Also the id could have a default value, don't see a reason why to require one.
  • OverlayView.position requires LatLng instance, the old lib was allowing LatLngLiteral which is easier to create, but no big deal I guess.

I might work on PRs tomorrow if desired.

@uriklar
Copy link
Collaborator Author

uriklar commented Apr 10, 2019

Hi @FredyC,
Thanks so much for the feedback! Sounds like pretty straight forward stuff. We should remove any non mandatory LoadScript props as mandatory.
I'm not sure I understand the use case for useGoogleMap though. The onLoad lets you access (and cache if needed) an instance of that specific component. useGoogleMap would only give you access to the map instance.

While you're here :-) I actually have a Typescript question that maybe you'll know the answer to:
I integrated this lib in my company's project that uses typescript (but also js) and was getting a module not found ts error that resolved only by adding moduleResolution: "node" to my tsconfig.json.

I'm using other libraries with direct (that is, not via DefinitelyTyped) TS support (for example styled-component) and they didn't require me to change the module resolution. Changing it makes my initial build suuuuper slower.

Got any ideas on that? I'm wondering if our tsconfig setup is correct... Should we be using rollup? I see you're using it on mobx-react-lite.

Thanks again!

@danielkcz
Copy link
Contributor

danielkcz commented Apr 10, 2019

I'm not sure I understand the use case for useGoogleMap though. The onLoad lets you access (and cache if needed) an instance of that specific component. useGoogleMap would only give you access to the map instance.

I need access to the map instance for panning purposes and often do that within components which are deeper in the tree. So the hook comes in really handy for that.


Regarding your typescript problem I am not sure really, but I am using moduleResolution: "node" pretty much everywhere. The other option is classic which is legacy anyway. Sounds strange it would slow down build so much, but never tried really.

I'm wondering if our tsconfig setup is correct

Well, you definitely have a fairly robust config, I usually get away with defaults and tweak some basics. I don't know half of that stuff what is it doing actually :) Might be worth investigating if you need all that.

Should we be using rollup?

I used it because I know it, but I heard lately good opinions about microbundle, might be worth the shot.

@JustFly1984
Copy link
Owner

@FredyC Thank you for improvements, I will publish new version tomorrow

@JustFly1984
Copy link
Owner

@FredyC Please add yourself to contributors list in /package/react-google-maps-api/package.json

@JustFly1984
Copy link
Owner

@FredyC can you please provide an example of useGoogleMap hook for docs and for gatsby-example?

@danielkcz
Copy link
Contributor

Please add yourself to contributors list in /package/react-google-maps-api/package.json

There is no list in that file :) Doesn't matter, it would be too hidden anyway, so I don't care.

can you please provide an example of useGoogleMap hook for docs and for gatsby-example?

Ok, hopefully by tomorrow I will able to stitch something together.

@mathieu-anderson
Copy link

mathieu-anderson commented Apr 16, 2019

Hello! We have started using this library for the implementation at my workplace (well, mostly me). As I understand it from perusing the issues, it looks like it's the case for most contributors.

I don't know yet if I will have the ability to contribute anything meaningful (still pretty junior), but I certainly would like to try.

And I am wondering if it would be desirable, on top of the private Slack channel, to maintain a Slack / Spectrum / Gitter channel for questions regarding specific use cases. I personally have a few, but as their place is probably not in the Issues, and the documentation / examples are very basic, and the library is pretty young, it's hard to find resources.

@danielkcz
Copy link
Contributor

I would kinda vote for Spectrum as well. Personally, I already have too many Slacks and want to limit that. Spectrum at least does not force to you have a separate set of credentials for each community. Although I don't exactly adore that platform just yet, it's still under heavy development and sometimes wonky. But it's a good alternative. The Gitter is useful for quick chatting too, but no separation per topic makes it useless for QA.

@uriklar
Copy link
Collaborator Author

uriklar commented Apr 16, 2019

Hi @mathieu-anderson.
Our Issues are indeed getting filled with usage questions.
Spectrum sounds like a great idea! It has the visibility that slack lacks and it is more specific the regular stackoverflow.

Here's a link to our newly created community: https://spectrum.chat/react-google-maps

Maybe your first PR could be adding it to the docs? :-)

@mathieu-anderson
Copy link

Lightning fast reaction time @uriklar ! I posted the first question on Spectrum!

And yes, I might look at adding to the docs! What's the process to make PRs to the project?

@shobishani
Copy link

HI @uriklar
I'm not sure if you should have asked this question here are hooks implemented in this library I need to access map Instance for some reasons in deep down tree.

@uriklar
Copy link
Collaborator Author

uriklar commented May 13, 2019

Hi @shobishani,
It's better you ask usage questions in our spectrum channel
We recently added the useGoogleMap hook that will return the map instasnce from anywhere in the tree. After successfully using it you can create a PR to add it to the documentation :-)

@shobishani
Copy link

@uriklar Thanks.. sure will submit PR

@janklimo
Copy link

janklimo commented Jun 5, 2019

Is anybody working on the implementation of InfoBox? We're working on migrating from react-google-maps so we could contribute to that guide once ready.

@JustFly1984
Copy link
Owner

@janklimo we are not working on InfoBox currently, if you want to make a PR, it is always welcome.
@uriklar do you have something to add?

@danielkcz
Copy link
Contributor

@JustFly1984 @uriklar Hey, do you know that latest 1.2.3 build is actually broken? In package.json you are referring to UMD folder, but it's not included in the build. Perhaps you haven't intended to release it yet and it should have been another alpha?

https://unpkg.com/@react-google-maps/api@1.2.3/

I would highly recommend not to use .npmignore, it's hard to maintain that in the long run. Instead, the files field in package.json is much easier (be it a whitelist). I will do PR to fix that.

@JustFly1984
Copy link
Owner

@FredyC Thank you!

@joaoreynolds
Copy link

@JustFly1984 @uriklar I'd like to take a crack at the OverlayView performance issues. But I don't have experience with lerna or storybook. I need some pointers on how to get started. I'm able to edit the storybook project and get it up and running, but what about making changes to /react-google-maps-api/src/components/dom/OverlayView.tsx - I'm not sure what to spin up so I can preview changes there as I develop.

Maybe add me to slack so I can talk with you there? I'll send an email to @uriklar

@uriklar
Copy link
Collaborator Author

uriklar commented Sep 25, 2019

Hi @joaoreynolds. Right now, the top issue preventing people from contributing is the lack of a good workflow for working on the library.
I can give you my workflow, but it's really terrible :-)
I think the solution needs to be setting up another package in the repo which will be a CRA app with typescript support. Then, this package could import directly from the react-google-maps-api folder instead of from the node_module. Or maybe using npm link or something like that.

Let me know if maybe you want to start from that. I'd be happy to assist on slack

@joaoreynolds
Copy link

I tried using npm link between storybook and react-google-maps-api, but that didn't seem to work. I'm comfortable setting up a CRA app. But what if we just changed the storybook project to import directly from the react-google-maps-api folder. It seems like a good place to do the development work. I suggest this because the storybook seems pretty empty right now, so I wonder what it's used for if not this...

Thoughts?

@JustFly1984
Copy link
Owner

The problem is that storybook does not support typescript. You can’t import TD files from storybook.

The solution will be to create separate package react-google-maps-api-cra-example with typescript enabled. This way we could import TD files

Another way is to add typescript to react-google-maps-api-gatsby-example

@joaoreynolds
Copy link

Ahh gotcha - okay I can create the cra example. I'll keep you updated if I have more questions.

@cuongdevjs
Copy link

Does ImageMapType supported?

@rbonigala
Copy link

Hi,
I am trying to develop a sample using the Next.Js, but the map is not getting rendered. Is there an example that I can refer to that has been implemented using NextJS framework.

@JustFly1984
Copy link
Owner

@rbonigala we have only gatsby-example in the repo, but in general SSR with next js should not be different than static rendering with Gatsby. Inshort - you can't render it in SSR, cos it requires DOM and injecting script tag in runtime. for SSR you can render fallback instead.

@mradenovic
Copy link
Contributor

mradenovic commented Oct 8, 2021

Hi @joaoreynolds. Right now, the top issue preventing people from contributing is the lack of a good workflow for working on the library. I can give you my workflow, but it's really terrible :-) I think the solution needs to be setting up another package in the repo which will be a CRA app with typescript support. Then, this package could import directly from the react-google-maps-api folder instead of from the node_module. Or maybe using npm link or something like that.

Let me know if maybe you want to start from that. I'd be happy to assist on slack

I find this very thru! I would like to give it a try. If someone can help with any working workflow, no matter how terrible it is, it would be great. I feel like How to test changes locally is outdated).

So far I have noticed the following issues, that might need new issues created:

  • Gatsby Example doesn't work. I've tried API Key that works with Docs.
  • yarn start:gatsby doesn't work out of the box.
  • Outdated docs (known issue Refresh the initial example on the NPM and the repo #2854).
  • Root CONTRIBUTING.MD references to nonexistent folders.
  • Root README.md doesn't contain much useful information.
  • /packages/react-google-maps-api/ README.md looks fairly otdated.

@iuliiaostapenko
Copy link

Hello guys. Maybe you know why documentation is down? https://react-google-maps-api-docs.netlify.app/ Maybe you could fix it, please :)

@vijay-t13
Copy link

Is anyone making documentation site up ?

@JustFly1984
Copy link
Owner

@vijay-t13 it is open source. If you want you can fix it. I will merge it and deploy. By now the config for docs is not updated to support latest current version, so docs build is broke. Somebody did a PR with downgrading docs to old version, and it is possible to run, but downgrading is not a good option. docs should be configured according new major version.

@iuliiaostapenko
Copy link

iuliiaostapenko commented Oct 10, 2023

Heads up, guys

When i run npm run build on the server I got error:

[commonjs--resolver] Failed to resolve entry for package "/var/www/Project/node_modules/@react-google-maps/api". The package may have incorrect main/module/exports specified in its package.json.
error during build:
Error: Failed to resolve entry for package "/var/www/Project/node_modules/@react-google-maps/api". The package may have incorrect main/module/exports specified in its package.json.
at packageEntryFailure (file:///var/www/Project/node_modules/vite/dist/node/chunks/dep-3bba9c7e.js:28730:11)
at resolvePackageEntry (file:///var/www/Project/node_modules/vite/dist/node/chunks/dep-3bba9c7e.js:28727:5)
at tryCleanFsResolve (file:///var/www/Project/node_modules/vite/dist/node/chunks/dep-3bba9c7e.js:28386:28)
at tryFsResolve (file:///var/www/Project/node_modules/vite/dist/node/chunks/dep-3bba9c7e.js:28333:17)
at Object.resolveId (file:///var/www/Project/node_modules/vite/dist/node/chunks/dep-3bba9c7e.js:28189:24)
at file:///var/www/Project/node_modules/rollup/dist/es/shared/node-entry.js:25544:40
at processTicksAndRejections (node:internal/process/task_queues:96:5)
at async PluginDriver.hookFirstAndGetPlugin (file:///var/www/Project/node_modules/rollup/dist/es/shared/node-entry.js:25444:28)
at async resolveId (file:///var/www/Project/node_modules/rollup/dist/es/shared/node-entry.js:24117:26)
at async ModuleLoader.resolveId (file:///var/www/Project/node_modules/rollup/dist/es/shared/node-entry.js:24531:15)

After I revert my version to previous in package.json all works just fine:
"@react-google-maps/api": "2.18.1"
My env-> react, ts, vite, inertia, laravel

@JustFly1984
Copy link
Owner

@iuliiaostapenko Do you want to became a contributor?

@JustFly1984
Copy link
Owner

JustFly1984 commented Oct 11, 2023

@iuliiaostapenko since now, accepting issues only with PR to fix it. There is no guys, I'm here alone maintaining this project.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests