Add rotation function

[shadowjustice]

• add event listeners but only return instance at this point in time
• add rotate animation to demo

This new pull request instead of #169 which is outdated and has merge conflicts.

[bumbu]

Hi @shadowjustice

Sorry for delayed response.
Thank you for the updated PR. I skimmed through the code and it looks good.
Also it's great that you added a demo.
There are few more things to do/check to make sure it will work as expected:

• Update Readme
• Add tests
• Make sure it works in supported browsers
  Other than that from looking at the code - it shouldn't break any existing apps which is great.

If you could update the PR with these details/changes - it will speed up merging this pull request. Otherwise I'll try to find some time during this weekend to go through these changes.

Thanks again for your work!

[shadowjustice]

I'll try to expand the readme tonight.

• Adding tests will be new for me (may take some time tho).
• I'll test several browsers also tonight

[deeankmet]

First of all, thank you for the MR and great work @shadowjustice.

I'm having issues with my rotation not happening around the center of the viewport.

I'm drawing a map from somewhat scattered xml-data but setting the parameters fit and center, makes it work. But when rotating, my map rotates away from the area instead of staying centered.

I've tried setting transform-box: fill-box for the viewport, that somewhat fixes the issue. But that creates other problems with the map not appearing centered at start for example.

Do you have any idea?

[shadowjustice]

First of all, thank you for the MR and great work @shadowjustice.

I'm having issues with my rotation not happening around the center of the viewport.

I'm drawing a map from somewhat scattered xml-data but setting the parameters fit and center, makes it work. But when rotating, my map rotates away from the area instead of staying centered.
Do you have any idea?

Hi Indeed atm the rotation is not around the center. I am going to check on how to set a point around which the svg rotates.

[shadowjustice]

First of all, thank you for the MR and great work @shadowjustice.

I'm having issues with my rotation not happening around the center of the viewport.

I'm drawing a map from somewhat scattered xml-data but setting the parameters fit and center, makes it work. But when rotating, my map rotates away from the area instead of staying centered.

I've tried setting transform-box: fill-box for the viewport, that somewhat fixes the issue. But that creates other problems with the map not appearing centered at start for example.

Do you have any idea?

@deeankmet I have researched it for a bit and realised the following.

For rotation to happen we add rotate
Rotate ( angle width, height)
the width and height decide the center point of the rotation.

So if you have a rectangle 50x50 the centerpoint is 25x25 so
Rotate (angle 25 25) would rotate around the centerpoint.
In the code the following happens:
angle: this.getRotate(), x: this.getViewBox().width / 2, y: this.getViewBox().height / 2

So my suspicion is that those x and y aren't correct in your case. Which I do not fully understand then because the height and width are the height and width from your viewport.

Anyways rotating around something differnt then the centerpoint could be seen as an improvement but secondary in my opinion. First getting this PR done.

[bumbu]

I'm drawing a map from somewhat scattered xml-data but setting the parameters fit and center, makes it work. But when rotating, my map rotates away from the area instead of staying centered.

This probably happens because the image is panned, but the rotate works from the middle. If that's the case then the centre of what user sees is not the same as the centre of current rotation, and the image is moved away. That's why for zoom the library by default zooms based on visible centre, and has another function to zoom in any point of the image (e.g. used for zooming by mouse scroll).

[shadowjustice]

That seems indeed the reason. Then maybe we should make the center point variable. Actually it should be variable. I’ll try and add it.

[deeankmet]

Thanks for the research @shadowjustice and @bumbu.

That makes sense. I've solved my issue by adding all my drawn elements to a group, then centrering the entire group on origo which makes the rotation work perfectly independent of the data I used.

Thanks again.

[shadowjustice]

I haven't been able to expand this. Any toughts on current commits?

[bumbu]

@shadowjustice sorry for the delay.
I wanted to quickly merge it today, but now after 2 hours spent on it, I'm stuck. What my thought process was:

• We should remove relative rotation as it's aditional API that's very easy to implement with get and set APIs
• Better to rename rotate to setCenterRotationAngle/getCenterRotationAngle to allow in the future to add setRelativeRotationAngle or even setRotation (as rotation can be defined by the angle and point of rotation)
• beforeRotate and onRotate will not work at all as they're not used in the shadow-viewport. They' d have to be hooked into the update lifecycle so that it can prevent the update or change it (which is the main benefit of those callbacks). Given how much effort it is, and probably lack of need of this functionality - I'd rather remove those
• This change breaks CSS animations (at least in Chrome). For some reason if a transform property gets the rotate, any code after that simply doesn't update the style of that element (which was introduced specifically for CSS animations).

This last point is kind of a deal breaker, and I'm not sure what to do about it.
I stashed my wip in f7c114a

[shadowjustice]

@shadowjustice sorry for the delay.
I wanted to quickly merge it today, but now after 2 hours spent on it, I'm stuck. What my thought process was:

• We should remove relative rotation as it's aditional API that's very easy to implement with get and set APIs
• Better to rename rotate to setCenterRotationAngle/getCenterRotationAngle to allow in the future to add setRelativeRotationAngle or even setRotation (as rotation can be defined by the angle and point of rotation)
• beforeRotate and onRotate will not work at all as they're not used in the shadow-viewport. They' d have to be hooked into the update lifecycle so that it can prevent the update or change it (which is the main benefit of those callbacks). Given how much effort it is, and probably lack of need of this functionality - I'd rather remove those
• This change breaks CSS animations (at least in Chrome). For some reason if a transform property gets the rotate, any code after that simply doesn't update the style of that element (which was introduced specifically for CSS animations).

This last point is kind of a deal breaker, and I'm not sure what to do about it.
I stashed my wip in f7c114a

Hi,

I had to reset my pc and work has been piling on. Does your wip contain an example for the bug in chrome?

I have been thinking about this change and for the main part while now I do
Matrix () Rotate() actually I should be using only matrix. Since you can also rotate with matrix. This will maybe solve the problem for chrome.

[bumbu]

@shadowjustice sorry for the delay.
I wanted to quickly merge it today, but now after 2 hours spent on it, I'm stuck. What my thought process was:

• We should remove relative rotation as it's aditional API that's very easy to implement with get and set APIs
• Better to rename rotate to setCenterRotationAngle/getCenterRotationAngle to allow in the future to add setRelativeRotationAngle or even setRotation (as rotation can be defined by the angle and point of rotation)
• beforeRotate and onRotate will not work at all as they're not used in the shadow-viewport. They' d have to be hooked into the update lifecycle so that it can prevent the update or change it (which is the main benefit of those callbacks). Given how much effort it is, and probably lack of need of this functionality - I'd rather remove those
• This change breaks CSS animations (at least in Chrome). For some reason if a transform property gets the rotate, any code after that simply doesn't update the style of that element (which was introduced specifically for CSS animations).

This last point is kind of a deal breaker, and I'm not sure what to do about it.
I stashed my wip in f7c114a

Hi,

I had to reset my pc and work has been piling on. Does your wip contain an example for the bug in chrome?

I have been thinking about this change and for the main part while now I do
Matrix () Rotate() actually I should be using only matrix. Since you can also rotate with matrix. This will maybe solve the problem for chrome.

The bug is reproducible both in my stack, and in this PR. You can see that the <g> element that receives the transform prop, doesn't have any style prop.

If we could rotate using matrix transform, that would solve the issue and unblock this PR from being merged.

[shadowjustice]

Okay before my changes we had a SVG transform attribute AND a style transform attribute filled in:

1. Master
   

So in my original code (not the reworked from @bumbu. Animation still works. Except for the fact that no css style attribute is present anymore:
2. Rotate (original PR)

For some reason after applying the changes from @bumbu (which in a sense are definitely good changes) the style property appears again but this time while it is present it does not update.

The reason for not updating has now been found and are more or less just problems with formatting and differences between svg transform and css transform.

I quote svg transform doc

Warning: As per the spec you should be able to also use CSS transform functions, however, the compatibility isn't guaranteed.

This link

Anyhow I come with the following question. I think this is a problem in itself.

The problem would be solved if we only use the svg transform since that one also has rotation around a certain point built in.. If we would use css transform then we need to make use of transform-origin to make rotations around center points possible.

Since I'm not sure how to put a stash on github

Thoughts you guys have? In my current version I have converted the matrix back to the seperate actions => scale, translate,... and separated the built up transform for css and svg. I have a working set.

Since I am not sure how to upload a stash to github in attachment the diff
file.txt

[ariutta]

Thanks for the pull request, and I apologize for my lack of response. My job has shifted away from front end, so I unfortunately can't keep up with maintenance. Would you be interested in becoming a maintainer?

#395