Skip to content

Option to position and size components with two corners. #580

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

Merged

Conversation

mtogstad
Copy link
Contributor

This lets you lock a component to a specific bounding rectangle, which allows for precise tiling during zoom with the experimental google maps API.

I work on a project that uses a technique similar to the thousands-of-markers sample project linked to from the readme. If you run that project against the 3.32 google maps API you can see the same issue I was having. The tiles are not scaled when frames are drawn during a zoom, which causes the edges to either have gaps between them, or to overlap. This adds the option to specify a second LatLng (the southeast corner) to give a component a bounds-locked box to render into.

Here is a before example:
giphy

And after:
giphy-1

This lets you lock a component to a specific bounding rectangle, which allows for precise tiling during zoom.
Copy link
Member

@itsmichaeldiego itsmichaeldiego left a comment

Choose a reason for hiding this comment

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

Good work man, let me test it and I will merge it!

if (
child.props.seLatLng !== undefined ||
(
child.props.seLat !== undefined &&
Copy link
Member

Choose a reason for hiding this comment

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

@mtogstad Could you explain briefly what are you doing here and why? Maybe leave a comment

@itsmichaeldiego
Copy link
Member

@stephenfarrar What do you think about this? Given its part of the code you've been working with.

@stephenfarrar
Copy link
Contributor

Looks good to me.

@mtogstad
Copy link
Contributor Author

Just pushed an explanation, and thanks for looking at this pull request so fast!

Copy link
Member

@itsmichaeldiego itsmichaeldiego left a comment

Choose a reason for hiding this comment

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

LGTM.

@itsmichaeldiego itsmichaeldiego merged commit 92b502b into google-map-react:master May 17, 2018
@itsmichaeldiego itsmichaeldiego mentioned this pull request May 17, 2018
@itsmichaeldiego
Copy link
Member

@mtogstad Thank you very much for this! Please update to 1.0.3 and let me know how it works. Feel free to make more PRS, we need more help!

@palashkaria
Copy link

palashkaria commented Jul 4, 2018

@mtogstad Hey, I know that this issue is closed, but I'm curious, how did this help with the canvas markers? I am using it for a similar use case & facing the exact same problem. Could you share the relevant change you made in this repo, https://github.com/istarkov/google-map-thousands-markers, which fixed the issue?

@itsmichaeldiego
Copy link
Member

@palashkaria Which version are you using? We're currently at 1.0.5

@palashkaria
Copy link

I'm on 1.0.5 itself, but the issue is not with this lib, it's with the canvas implementation (https://github.com/istarkov/google-map-thousands-markers).

What I understood from this change is that I have to pass seLatLng to CanvasTile in that implementation. But that doesn't seem to work..

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Jul 4, 2018

@palashkaria Oh yes, what changed is that you can pass either

seLatLng={[lat, lng]}

or

seLat={lat} seLng={lng}

@palashkaria
Copy link

palashkaria commented Jul 4, 2018

I think it's like seLatLng={{lat, lng}}. But question is, specifically how does it help in those before/after images posted above, from the 'google-map-thousands-markers' example. I'm trying something here, but it doesn't seem to work..

palashkaria/google-map-thousands-markers@500dacf#diff-4848da3e28fb800ab253319bb2777f2e

@itsmichaeldiego
Copy link
Member

@palashkaria That should work given @mtogstad's change, but there might be a chance that you dont need that seLatLng anymore as we went through some interesting changes in the way markers are drawn. What exactly you want to accomplish? Could you provide a live example?

I think the best person to answer would be @mtogstad, as he did the change and tested it locally.

@palashkaria
Copy link

@itsmichaeldiego here's an updated codesandbox version of istarkov's example, which also has the same issue. You can check it out here:

https://codesandbox.io/s/kx7lo2plq5
https://kx7lo2plq5.codesandbox.io/

This also has the seLatLng change that I made in src/CanvasMap. It'd be helpful if you/ @mtogstad could take a look & figure out what's going wrong here

@punnone
Copy link

punnone commented Jul 19, 2018

@palashkaria I'm facing the same problem. Did you get a solution?

@Heedster
Copy link

@punnone [I work with @palashkaria] No. We are still trying to figure it out.

@Heedster
Copy link

Heedster commented Jul 21, 2018

Fixed it. 2 things have to be done.

  1. Adding a seLatLng to the tile [CanvasMap.js]
tiles[key] = {
        ...tile2LatLng({ x, y }, zoom),
        seLatLng: { ...tile2LatLng({ x: x + 1, y: y + 1 }, zoom) },
        x,
        y,
        markers: []
   };
  1. Remove explicit width from the canvas tile [CanvasTile.js]
const canvasStyle = {
  width: "100%",
  height: "100%"
};

Updated the codesandbox for reference: https://codesandbox.io/s/6v5vx7z65w

@lock
Copy link

lock bot commented Dec 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants