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

Add support leaflet for v1.9.3 #21

Merged
merged 9 commits into from
Jan 27, 2023
Merged

Add support leaflet for v1.9.3 #21

merged 9 commits into from
Jan 27, 2023

Conversation

Raruto
Copy link
Owner

@Raruto Raruto commented Jan 20, 2023

Compare latest changes from: Leaflet/Leaflet@v1.7.0...v1.9.3

// paddingBottomRight: size.subtract(anchor)
// });
},

Copy link
Owner Author

@Raruto Raruto Jan 20, 2023

Choose a reason for hiding this comment

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

@siimaoo the reason behind that bug should be this new feature marker-autopanonfocus (added by leaflet v1.8).

For the moment I have disabled it, could you help me in trying to re-enable it?

If you take a quick look to the rest of this plugin code in general it's just about "adding" a rotation function (to a point or bounds calculation) among those already present in this library, the difficult thing is enough time right to find the right one..

Ref: #18

Copy link

Choose a reason for hiding this comment

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

tonight (gmt -3) I'll take a look at this

test/index.html Outdated
<!-- Leaflet-MarkerCluster -->
<script src="https://unpkg.com/[email protected]/dist/leaflet.markercluster-src.js"></script>
<link rel="stylesheet" href="https://unpkg.com/[email protected]/dist/MarkerCluster.css">
<link rel="stylesheet" href="https://unpkg.com/[email protected]/dist/MarkerCluster.Default.css">

Copy link
Owner Author

Choose a reason for hiding this comment

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

@siimaoo for your tests, here is a complete example of integration with leaflet v1.9 and leaflet.markercluster

Copy link

@siimaoo siimaoo Jan 23, 2023

Choose a reason for hiding this comment

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

cluster.bug.mp4

the bug persist!

00:00 - 00:30 - everything ok
00:30 - 00:45 - bug

Copy link
Owner Author

Choose a reason for hiding this comment

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

@siimaoo are you sure you're not running your tests on the wrong file? (ie: examples/index.html instead of test/index.html)

The code of the leaflet-v1.9.3 branch should also show you some other info about the pixel bounds (a red-dotted rectangle in the map and a related table row, like the picture shown here #18 (comment))

Copy link

Choose a reason for hiding this comment

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

@Raruto sorry, I running the test on the wrong file! Now, everything is good!

Copy link

Choose a reason for hiding this comment

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

Is working with cluster, draggable markers and popups! Nice!


// @option bounceAtZoomLimits: Boolean = true
// Set it to false if you don't want the map to zoom beyond min/max zoom
// and then bounce back when pinch-zooming.
bounceAtZoomLimits: false,
Copy link
Owner Author

Choose a reason for hiding this comment

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

@heyajohnny looks so much like a duplicate of:

bounceAtZoomLimits: true,

BTW, could be related to #19? (eg. due to current incorrect map bounds calculations, ref: #18 (comment))

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it's a duplicate of 'TouchGestures.js' and this one (in 'TouchZoom.js') can be removed.
I've tested it with these combinations:

  1. touchGestures: true, touchZoom: false
  2. touchGestures: false, touchZoom: true
  3. touchGestures: true, touchZoom: true

And it works as expected without this 'bounceAtZoomLimits: false,'.

@@ -63,7 +63,7 @@ L.Map.TouchGestures = L.Handler.extend({

this._moved = false;

map.stop();
map._stop();
Copy link
Owner Author

Choose a reason for hiding this comment

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

@heyajohnny did you happen to find the original source of the L.Map.TouchGestures.js handler ? (I could be wrong, but I don't think I developed it), although I don't think there's anything like it in here:

I'm just asking because it seems strange that a different method has been used here before than the original implementation (ie map.stop() instead of map._stop(), ref: fnicollet/Leaflet/src/map/handler/Map.TouchZoom.js#L52)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, Your first version contained the map.stop(), but all the other current handlers have the map._stop(), so I guessed it was either a typo or an old method that changed name.

Copy link
Owner Author

Choose a reason for hiding this comment

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

@heyajohnny If it helps, this is the commit where it was initially added: fnicollet/Leaflet@a77af51

🤦‍♂️ I couldn't find it because I was looking in the wrong branch.. (master instead of rotate-master).

I had also written it in the readme file:

Side notes:

Be aware that this is a proof of concept that shows how to alter the core leaflet library and make rotation feature usable as a standalone plugin. Check out fnicollet's rotate-master branch if you prefer to use a reliable all-in-one solution.

Latest changes to this project have been updated comparing on the following: Leaflet/Leaflet@main...fnicollet:rotate-master (ref: bac6c7d)

internally replace `this.getPixelBounds()` call with `this._container.getBoundingClientRect()` for determing outer map container pixel coordinates
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.

3 participants