Update rotate branch + Add improvements #7217
Conversation
ronikar
changed the title
|
Hi @Keats, |
|
I ran into some weird edge cases when rotating that were not obvious to debug so I haven't really used my branch. |
|
Hi @johnd0e, |
src/dom/DomUtil.js
Outdated
| @@ -210,28 +212,37 @@ export function testProp(props) { | |||
| // Resets the 3D CSS transform of `el` so it is translated by `offset` pixels | |||
| // and optionally scaled by `scale`. Does not have an effect if the | |||
| // browser doesn't support 3D CSS transforms. | |||
| export function setTransform(el, offset, scale) { | |||
| export function setTransform(el, offset, scale, bearing, pivot) { | |||
There was a problem hiding this comment.
Too many arguments, here and in setPosition.
May be it'd better to handle all rotation-related stuff separately.
Like, store them as properties, and apply automatically.
There was a problem hiding this comment.
It is based on the current version
There was a problem hiding this comment.
I understand that you've only updated existing rotate branch.
Still I do not like that part.
There was a problem hiding this comment.
What do you mean by "handle all rotation-related stuff separately". Could you please expand on that? I don't quite understand what you mean by that.
There was a problem hiding this comment.
Just for example separate function setRotation.
There was a problem hiding this comment.
Well, I'm not sure that current implementation is convenient enough.
I do not like things like DomUtil.setPositionAndRotation.
I'd consider another approach: what if setRotation will just put _leaflet_rotate property into html el?
And DomUtil.setPosition will use that property to keep that rotation.
This is just an idea, I haven't reviewed all the things as a whole!
There was a problem hiding this comment.
I don't like that approach, I prefer to pass bearing as parameter. It can be very confusing
src/dom/Draggable.js
Outdated
| @@ -49,12 +49,14 @@ export var Draggable = Evented.extend({ | |||
|
|
|||
| // @constructor L.Draggable(el: HTMLElement, dragHandle?: HTMLElement, preventOutline?: Boolean, options?: Draggable options) | |||
| // Creates a `Draggable` object for moving `el` when you start dragging the `dragHandle` element (equals `el` itself by default). | |||
| initialize: function (element, dragStartTarget, preventOutline, options) { | |||
| initialize: function (element, dragStartTarget, preventOutline, options, map, anchor) { | |||
There was a problem hiding this comment.
Too many arguments.
May be better put them into options.
Also I have concerns about some edge cases: what if draggable is detached from initial map and reattached to another in run time?
There was a problem hiding this comment.
There is a problem when draggable is reattached to another map with a different angle.
|
Those are just preliminary comments, as I haven't checked your PR main functionality. |
|
Just to be clear, my goal is to update rotate branch , not |
|
Good to see that some people are still working on this branch. Thanks! |
|
Hi, is it possible to also consider tilt change? vector tiles can be 3d. Take a look at https://www.google.com/search?q=leaflet+bearing+and+tilt&oq=leaflet+bearing+and+tilt&aqs=chrome..69i57.2970j0j7&sourceid=chrome&ie=UTF-8 also |
|
@Azbesciak, I don't know how to consider tilt change because |
|
I created leaflet-rotate-map package for those who want to use the latest leaflet version with rotation :) |
|
closed? so..its merged?. Thanks! |
No. |
|
Fixes #Hi,
In The last year, I have worked with that repository, with
rotatebranch. In the last week I decided to update my local version (based 1.3.2) to the latest leaflet release #268. In the last time, I didn't know how to work withpull requestsconcept, but now, I will be more than happy to contribute some improvements that I did.Summary of the changes:
The pull request is from my local branch to
master, because I made a rebase