Skip to content

Centers icons not on top left corner but on pin point.#10

Open
FabienTregan wants to merge 1 commit intodevfriendlyplaces:masterfrom
FabienTregan:corrects-icon-position
Open

Centers icons not on top left corner but on pin point.#10
FabienTregan wants to merge 1 commit intodevfriendlyplaces:masterfrom
FabienTregan:corrects-icon-position

Conversation

@FabienTregan
Copy link

The icons where positionned from top-left corner, causing two problems :
-The position was not completely accurate
-The icons seemed to move when you zoom in/out

This patch works but the position is hardcoded.

@avernois
Copy link
Contributor

thanks for the contribution.

sorry for the long delay, that project is not on top of my priority list (and this PR slip out of my mind :(

I took me time to understand the problem you were trying to solve. I think before submitting a PR, opening an issue with all available detail (maybe picture as this one is a visual issue) is the best thing to do. (This is for a next time, this one is done, don't bother :)

{ icon: L.icon(
{ iconUrl: getMarkerImage({wifi: place.wifi, power: place.power}) }
{ iconUrl: getMarkerImage({wifi: place.wifi, power: place.power}),
iconAnchor: [12, 41]}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like these magic number that seems to come from nowhere.
I think a simple solution would be to give them a name (or a single name for the couple ?) by extracting them as variables or constants.
Another one solution could be to compute them from the image (as I guess they represent its center). But that might be overworked (as all icon have the same size and we have no plan to changing them).
An in between solution could be to add a method that, given the size of an image returns the coordinates of the center (but we then have to solve the problem of where does the image size come from :)

So I'll probably start with the first solution and see how it works from here.

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.

2 participants