search

IITC-CE / Leaflet.Canvas-Markers

Leaflet plugin for displaying icons on canvas instead of DOM
MIT License
2 stars 4 forks source link

Changes for IITC #1

Closed modos189 closed 5 years ago

modos189 commented 5 years ago

On one hand, this change is specific to IITC, and on other hand - for greater similarity with the leaflet. Perhaps we should create a parameter to determine whether the container should be destroyed. See: https://github.com/IITC-CE/Leaflet.Canvas-Markers/commit/b6c228a1b1752255c6b82be3c91473fc02b578af

johnd0e commented 5 years ago

https://github.com/IITC-CE/Leaflet.Canvas-Markers/pull/1/commits/a52cbaf383b38e1c642bc5d4e34ae399688e8ba5

Nice job!

modos189 commented 5 years ago

Maybe you'll be able to finish options.padding support. Ornaments are constantly shifting: either by moving the map or changing the screen size

johnd0e commented 5 years ago

Ornaments are constantly shifting

We need padding to solve this, or this issue caused by unfinished padding support?

modos189 commented 5 years ago

It's not ready. Maybe I should have sent it to a separate branch. Padding is needed to draw portals that are outside the visible part of the screen, but close to it, just like in Leaflet and IITC. I have implemented this, but there are some conflicts when moving the map. Ornaments move more than portals.

johnd0e commented 5 years ago

As this feature is not blocker it makes sense to put it separate branch.

And tell me more about 419b1ab: what is 'atypical use'?

modos189 commented 5 years ago

I was able to fix it. Padding works correctly.

what is 'atypical use'?

Our code tries to remove those markers which may not exist.

https://github.com/IITC-CE/ingress-intel-total-conversion/blob/256f9c53a866a254b40e40b1f766e58b4d2a89f4/code/ornaments.js#L69-L73

That's why I added additional checks to correct the errors.

johnd0e commented 5 years ago

I was able to fix it. Padding works correctly.

Great! But check upper comment please ^^^:

johnd0e commented 5 years ago

Our code tries to remove those markers which may not exist.

removeMarker was broken anyway.

Also I am going to fix our code, so we do not need additional checks in upstream.

Although, those checks may make sense in general (but not in this branch).

johnd0e commented 5 years ago

@modos189 I am going to clean up and rewrite this branch, tell me when you ready. Separate branches for all features already created (todo: a52cbaf383b38e1c642bc5d4e34ae399688e8ba5)

johnd0e commented 5 years ago

Please check comments regarding padding option. I have implemented fixes in padding-option branch, but not tested yet.

johnd0e commented 5 years ago

My fault, thank you for pointing out.

modos189 commented 5 years ago

I saw you similar change in padding-option branch. But there is an error: instead of layerPointToContainerPoint you should use containerPointToLayerPoint

johnd0e commented 5 years ago

I saw you similar change in padding-option branch

I've just updated that branch with your last commit. Let's continue padding-related development there.

johnd0e commented 5 years ago

As all needed for IITC features are now separated between own branches, all we need - merge them together, like this:

git checkout master
git checkout -b changes-for-IITC
git merge --no-ff fixes canvas-resize layer-features-back padding-option
git push --force

I've already done this in test branch, and about to rewrite changes-for-IITC branch with the content of test.

All future updates should go to feature branches, here we'll need merge them together only.

P.S. And I suppose we need to open corresponding PR's in Spaction's repo.

johnd0e commented 5 years ago

@modos189 I finished with all branches except padding-option.

I am unable to check it, as I just do not understand how it should function. Let's start from related issue: https://github.com/IITC-CE/ingress-intel-total-conversion/issues/184

modos189 commented 5 years ago

It's all working now.

johnd0e commented 5 years ago

@modos189 I am not sure that 2 latest commits are needed. See test branch (as I said in previous message)

modos189 commented 5 years ago

In https://github.com/IITC-CE/ingress-intel-total-conversion/pull/181 which branch is being used? I used that version and I thought it was from test branch

up: Indeed, these changes already exist in the test branch

johnd0e commented 5 years ago

Do you still need this branch? If not then I will rewrite it with actual commits.

modos189 commented 5 years ago

No, you can rewrite

johnd0e commented 5 years ago

Please check this comment: https://github.com/IITC-CE/Leaflet.Canvas-Markers/commit/71eab756484bf298384f5059f15d2fd86fb0b182#commitcomment-33502466

modos189 commented 5 years ago

I'm totally confused in branches, so I created a new branch for onresize method: https://github.com/IITC-CE/Leaflet.Canvas-Markers/tree/onresize

Merge after this: https://github.com/IITC-CE/Leaflet.Canvas-Markers/pull/1#issuecomment-489002729

johnd0e commented 5 years ago

I'm totally confused in branches

We have independent branch for every new feature here. We keep it so it will be easy to make PR's to parent repo in the future.

And all that branches merge together here, in changes-for-IITC - it is not independent branch.

So to complete our work here we should finish last branch, for what we have concerns: padding-option. I think it is not ok, check comments for f451114f2f74e9cd896cd8a160e3d34bffdcc7ff

modos189 commented 5 years ago

@johnd0e https://github.com/IITC-CE/Leaflet.Canvas-Markers/commit/1d88dcfee3a83e5ddd3e41abab6ed3f996623bb7

johnd0e commented 5 years ago

@johnd0e 1d88dcf

Good! But see question there.

Also question about _updateTransform left unanswered in https://github.com/IITC-CE/Leaflet.Canvas-Markers/commit/f451114f2f74e9cd896cd8a160e3d34bffdcc7ff