ADD: getAngle() method #480
Conversation
Resolves #102
|
Seems good to me. |
|
Can we write a test here ? |
|
Hi @SidharthBansal. Thank you for approving the task. This HOF task seems to be redundant. |
|
looking now! thanks guys |
There was a problem hiding this comment.
this is so great!! Just some refactoring stuff and we can merge thank you so much!
also, remember to run $ grunt concat:dist so that your changes get concatenated into the dist file (dist/leaflet.distortableimage.js) and commit that file here too.
Easy way to remember that part is to just run $ grunt in a separate terminal tab as you make changes and it'll update it for you automatically
src/DistortableImageOverlay.js
Outdated
| @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({ | |||
| return this; | |||
| }, | |||
|
|
|||
| getAngle: function() { | |||
| var matrix = this._image.style.transform | |||
There was a problem hiding this comment.
small refactor to use Leaflet helper methods:
| var matrix = this._image.style.transform | |
| var matrix = L.DomUtil.getStyle(this.getElement(), 'transform') |
src/DistortableImageOverlay.js
Outdated
| .slice(1, -1) | ||
| .split(','); | ||
|
|
||
| var angle = Math.round(Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI)); |
There was a problem hiding this comment.
Any specific reason to round here? If not, lets just leave it unrounded?
| var angle = Math.round(Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI)); | |
| var angle = Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI); |
|
Hi isn't it a hard issue?
If it is fixed, please close it
…On Sun, 22 Dec 2019, 7:27 am Sasha Boginsky, ***@***.***> wrote:
***@***.**** requested changes on this pull request.
this is so great!! Just some refactoring stuff and we can merge thank you
so much!
also, remember to run $ grunt concat:dist so that your changes get
concatenated into the dist file (dist/leaflet.distortableimage.js) and
commit that file here too.
Easy way to remember that part is to just run $ grunt in a separate
terminal tab as you make changes and it'll update it for you automatically
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
+ .split('matrix3d')[1]
+ .slice(1, -1)
+ .split(',');
+
+ var angle = Math.round(Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI));
+
+ if (angle < 0) {
+ angle = 360 + angle;
+ }
+
+ return angle;
so that -0 is never returned!
⬇️ Suggested change
- return angle;
+ return Math.abs(angle);
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
small refactor to use Leaflet helper methods:
⬇️ Suggested change
- var matrix = this._image.style.transform
+ var matrix = L.DomUtil.getStyle(this.getElement(), 'transform')
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
+ .split('matrix3d')[1]
+ .slice(1, -1)
+ .split(',');
+
+ var angle = Math.round(Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI));
Any specific reason to round here? If not, lets just leave it unrounded?
⬇️ Suggested change
- var angle = Math.round(Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI));
+ var angle = Math.atan2(matrix[1], matrix[0]) * (180 / Math.PI);
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
+ .split('matrix3d')[1]
We want to add 2 extra spaces to each of these so that they're lined up
with "matrix" and the linter doesn't complain:
⬇️ Suggested change
- .split('matrix3d')[1]
+ .split('matrix3d')[1]
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
+ .split('matrix3d')[1]
+ .slice(1, -1)
⬇️ Suggested change
- .slice(1, -1)
+ .slice(1, -1)
------------------------------
In src/DistortableImageOverlay.js
<#480 (comment)>
:
> @@ -274,6 +274,21 @@ L.DistortableImageOverlay = L.ImageOverlay.extend({
return this;
},
+ getAngle: function() {
+ var matrix = this._image.style.transform
+ .split('matrix3d')[1]
+ .slice(1, -1)
+ .split(',');
⬇️ Suggested change
- .split(',');
+ .split(',');
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#480?email_source=notifications&email_token=AFAAEQ4WH5F4XBKKMFAEL6TQZ3CRLA5CNFSM4J55G4S2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQAUUCA#pullrequestreview-335628808>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFAAEQ5MJ6F625RIJNQ5YQDQZ3CRLANCNFSM4J55G4SQ>
.
|
|
@SidharthBansal @VladimirMikulic Also: one more thing - if we flip the image upside down by dragging the top 2 corners lower than the bottom 2, the rotation becomes incorrect. I was thinking maybe the solution would be to do: var row0x = matrix[0];
var row0y = matrix[1];
var row1x = matrix[4];
var row1y = matrix[5];
var determinant = row0x * row1y - row0y * row1x;
var angle = Math.atan2(row0y, row0x) * (180 / Math.PI);
if (determinant < 0) {
angle += angle < 0 ? 180 : -180;
}
if (angle < 0) {
angle = 360 + angle;
}
// ignore my Math.abs(angle) suggestion earlier
return angle;@VladimirMikulic can you confirm if you think this is correct or not |
|
@sashadev-sky you were right. When we flip the image my implementation shows 360 instead of 180. Also, thanks for clarifying the difference between |
The old implementation of getAngle() method would show the incorrect angle when we flip the image by dragging the 2 top corners below the bottom 2 corners. This change resolves it.
|
Resolved |
The video shows the method in action.
getAngle()returns a value in degrees(0 - 360).Resolves #102
@keshav234156 @jywarren @rexagod could someone review this?
Thanks.