ADD: setAngle() method #502

VladimirMikulic · 2019-12-27T01:21:49Z

The method accepts an angle in degrees and sets element's rotation to be
that angle. It can also accept negative degrees.

Demo

Resolves #102

@sashadev-sky @jywarren @SidharthBansal could you review this, please?

The method accepts an angle in degrees and sets element's rotation to be that angle. Resolves #102

jywarren · 2020-01-02T18:08:10Z

This is awesome. Would you be interested in making a simple test for this? Thank you!

VladimirMikulic · 2020-01-02T21:05:13Z

@jywarren this cannot be tested because getAngle() method uses L.DomUtil.getStyles function which is undefined(not accessible) in tests. I could refactor getAngle() and then add tests in separate PR.

Can we get this feature merged?
Thanks.

jywarren · 2020-01-02T21:07:51Z

Sure, we'll go ahead and merge this then; it's an addition rather than a change in any case. Would you be able to open a new issue describing what'd have to happen to get a test for this? Thank you!

VladimirMikulic · 2020-01-02T22:05:47Z

@jywarren thanks! I could open the issue, that's not the problem, but the problem is in tests.
In browser getAngle works flawlessly, but in the tests, it always returns 0.
It might be related to the fact that we use PhantomJS instead of a "real" browser.

We'll need to debug this issue before we could write a test for setAngle and getAngle.

jywarren · 2020-01-02T22:32:18Z

OK, that's fine - can you open an issue with that text? That way someone may come along who can help us resolve it. Thank you!

…

On Thu, Jan 2, 2020 at 5:05 PM Vladimir Mikulic ***@***.***> wrote: @jywarren <https://github.com/jywarren> thanks! I could open the issue, that's not the problem, but the problem is in tests. In browser getAngle works flawlessly, but in the tests, it always returns 0. It might be related to the fact that we use PhantomJS in tests instead of a "real" browser. We'll need to debug this before we could write a test for this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502?email_source=notifications&email_token=AAAF6JZRXWJUBFYAU45CSCDQ3ZQLZA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH7STNQ#issuecomment-570370486>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J3DDJTYU27FA4VERTDQ3ZQLZANCNFSM4J7TCU2Q> .

VladimirMikulic · 2020-01-02T23:45:53Z

@jywarren I have some news for you.
I was right, it was an issue with PhantomJS, L.DomUtil.getStyles is accessible in ChromeHeadless but inaccessible in PhantomJS. On the other hand, 3 tests fail on ChromeHeadless.

I've noticed that rotation property is correct although getAngle returns 0.
Maybe our test could be:

describe('#setAngle', function() {
    it('should set image angle to 90 deg', function () {
      overlay.setAngle(90)
      expect(overlay.rotation).to.equal(-90);
    });
  });

This would require fixing 3 failing tests, and switching to ChromeHeadless.
Also, PhantomJS is not active anymore.
What do you think? Thanks.

jywarren · 2020-01-03T17:12:10Z

That would be great -- shall we make a GCI issue for it, do you think? Are you participating in GCI? Just checking!

…

On Thu, Jan 2, 2020 at 6:50 PM Vladimir Mikulic ***@***.***> wrote: @jywarren <https://github.com/jywarren> I have some news for you. I was right, it was an issue with PhantomJS, L.DomUtil.getStyles is accessible when browser is ChromeHeadless, but inaccessible in PhantomJS. On the other hand, 3 tests fail on ChromeHeadless. I've noticed that rotation property is correct although getAngle returns 0. Maybe our test could be: describe('#setAngle', function() { it('should set image angle to 90 deg', function () { overlay.setAngle(90) expect(overlay.rotation).to.equal(-90); }); }); This would require fixing 3 failing tests, and switching to ChromeHeadless. Also, PhantomJS is not active <https://phantomjs.org/> anymore. What do you think? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502?email_source=notifications&email_token=AAAF6J6JQPHJIUXELEG4LO3Q3Z4DFA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH72PTQ#issuecomment-570402766>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAF6J46EJDKISPDIVNDWXTQ3Z4DFANCNFSM4J7TCU2Q> .

VladimirMikulic · 2020-01-03T17:17:17Z

Yes, I am participating in GCI and we could make GCI task out of this.
Could you also please provide your opinion on publiclab/plots2#7086.
It's form validation, very important :)

Thanks.

SidharthBansal · 2020-01-03T17:17:48Z

Vladimir is a gci student On Fri, 3 Jan 2020, 10:42 pm Jeffrey Warren, <notifications@github.com> wrote:

…

That would be great -- shall we make a GCI issue for it, do you think? Are you participating in GCI? Just checking! On Thu, Jan 2, 2020 at 6:50 PM Vladimir Mikulic ***@***.***> wrote: > @jywarren <https://github.com/jywarren> I have some news for you. > I was right, it was an issue with PhantomJS, L.DomUtil.getStyles is > accessible when browser is ChromeHeadless, but inaccessible in PhantomJS. > On the other hand, 3 tests fail on ChromeHeadless. > > I've noticed that rotation property is correct although getAngle returns > 0. > Maybe our test could be: > > describe('#setAngle', function() { > it('should set image angle to 90 deg', function () { > overlay.setAngle(90) > expect(overlay.rotation).to.equal(-90); > }); > }); > > This would require fixing 3 failing tests, and switching to ChromeHeadless. > Also, PhantomJS is not active <https://phantomjs.org/> anymore. > What do you think? Thanks. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > < #502?email_source=notifications&email_token=AAAF6J6JQPHJIUXELEG4LO3Q3Z4DFA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH72PTQ#issuecomment-570402766 >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAAF6J46EJDKISPDIVNDWXTQ3Z4DFANCNFSM4J7TCU2Q > > . > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502?email_source=notifications&email_token=AFAAEQ6DAE3Z7FF6XS6NCM3Q35WWXA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBTK7Y#issuecomment-570635647>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAAEQ3YJGP5WQO5MST624DQ35WWXANCNFSM4J7TCU2Q> .

SidharthBansal · 2020-01-03T17:18:35Z

Vladimir you can take the task which Jeff wants a GCI task if you want On Fri, 3 Jan 2020, 10:47 pm Sidharth Bansal, <bansal.sidharthcode@gmail.com> wrote:

…

Vladimir is a gci student On Fri, 3 Jan 2020, 10:42 pm Jeffrey Warren, ***@***.***> wrote: > That would be great -- shall we make a GCI issue for it, do you think? Are > you participating in GCI? Just checking! > > On Thu, Jan 2, 2020 at 6:50 PM Vladimir Mikulic ***@***.*** > > > wrote: > > > @jywarren <https://github.com/jywarren> I have some news for you. > > I was right, it was an issue with PhantomJS, L.DomUtil.getStyles is > > accessible when browser is ChromeHeadless, but inaccessible in > PhantomJS. > > On the other hand, 3 tests fail on ChromeHeadless. > > > > I've noticed that rotation property is correct although getAngle returns > > 0. > > Maybe our test could be: > > > > describe('#setAngle', function() { > > it('should set image angle to 90 deg', function () { > > overlay.setAngle(90) > > expect(overlay.rotation).to.equal(-90); > > }); > > }); > > > > This would require fixing 3 failing tests, and switching to > ChromeHeadless. > > Also, PhantomJS is not active <https://phantomjs.org/> anymore. > > What do you think? Thanks. > > > > — > > You are receiving this because you were mentioned. > > Reply to this email directly, view it on GitHub > > < > #502?email_source=notifications&email_token=AAAF6J6JQPHJIUXELEG4LO3Q3Z4DFA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEH72PTQ#issuecomment-570402766 > >, > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AAAF6J46EJDKISPDIVNDWXTQ3Z4DFANCNFSM4J7TCU2Q > > > > . > > > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#502?email_source=notifications&email_token=AFAAEQ6DAE3Z7FF6XS6NCM3Q35WWXA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBTK7Y#issuecomment-570635647>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AFAAEQ3YJGP5WQO5MST624DQ35WWXANCNFSM4J7TCU2Q> > . >

VladimirMikulic · 2020-01-03T17:19:44Z

Thanks @SidharthBansal, I've noticed that you approved the task on GCI dashboard.
Did you see my comments in the PR?
Could you tell what you think?

Thanks.

SidharthBansal · 2020-01-03T17:26:26Z

Hi I approved it when Jeff was at holidays and you have completed it. You were in blocked state(/free) so I wanted you to utilize your time productively in his and Sashas absence. You will get points for only completed tasks. Kindly complete your tasks to get their points.

…

On Fri, 3 Jan 2020, 10:49 pm Vladimir Mikulic, ***@***.***> wrote: Thanks @SidharthBansal <https://github.com/SidharthBansal>, I've noticed that you approved the task on GCI dashboard. Did you see my comments in the PR? Could you tell what you think? Thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#502?email_source=notifications&email_token=AFAAEQ24VEZVSVIYYATDC6DQ35XTDA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBT3IA#issuecomment-570637728>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFAAEQ4M6FRLC6FYHC66XYTQ35XTDANCNFSM4J7TCU2Q> .

SidharthBansal · 2020-01-03T17:27:33Z

I have specialisation in MK SWB and Plots. For LDI please refer other mentors. Thanks On Fri, 3 Jan 2020, 10:56 pm Sidharth Bansal, <bansal.sidharthcode@gmail.com> wrote:

…

Hi I approved it when Jeff was at holidays and you have completed it. You were in blocked state(/free) so I wanted you to utilize your time productively in his and Sashas absence. You will get points for only completed tasks. Kindly complete your tasks to get their points. On Fri, 3 Jan 2020, 10:49 pm Vladimir Mikulic, ***@***.***> wrote: > Thanks @SidharthBansal <https://github.com/SidharthBansal>, I've noticed > that you approved the task on GCI dashboard. > Did you see my comments in the PR? > Could you tell what you think? > > Thanks. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#502?email_source=notifications&email_token=AFAAEQ24VEZVSVIYYATDC6DQ35XTDA5CNFSM4J7TCU22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIBT3IA#issuecomment-570637728>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AFAAEQ4M6FRLC6FYHC66XYTQ35XTDANCNFSM4J7TCU2Q> > . >

sashadev-sky · 2020-01-09T05:09:48Z

@VladimirMikulic this is so great!!
Also I know that the Leaflet js library still uses phantomJS so i'm not sure we need an update unless you really want to make a case for it.
And sounds good for the rotation property. I'll open a new Github issue for that now and upload it to the GCI dashboard!

sashadev-sky · 2020-01-09T05:24:04Z

actually wait, the rotation property does not work correctly! I will update it to use your getAngle method. For the tests then we'd have to sort of polyfill L.DomUtil.getStyle? Anyway for GCI credit this is complete so don't worry about it for now

VladimirMikulic · 2020-01-09T11:56:35Z

@sashadev-sky we can either polyfill L.DomUtil.getStyle or switch to ChromeHeadless.

sashadev-sky · 2020-01-10T06:37:42Z

@VladimirMikulic I could be misunderstanding the problem you are running into but I think the issue might be that the css transform property throws the error, not the method, and it can be fixed by updating the code to var matrix = this._image.style[L.DomUtil.TRANSFORM]. Lmk!

VladimirMikulic · 2020-01-10T12:16:02Z

@sashadev-sky yes, this._image.style[L.DomUtil.TRANSFORM] doesn't throw errors in the test, but the returned angle from getAngle is still 0.

ADD: setAngle() method

c0a517f

The method accepts an angle in degrees and sets element's rotation to be that angle. Resolves #102

jywarren merged commit ab3eccc into publiclab:main Jan 2, 2020

sashadev-sky added in progress question and removed in progress labels Jan 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD: setAngle() method #502

ADD: setAngle() method #502

VladimirMikulic commented Dec 27, 2019 •

edited

Loading

jywarren commented Jan 2, 2020

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

jywarren commented Jan 2, 2020

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

jywarren commented Jan 2, 2020 via email

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

jywarren commented Jan 3, 2020 via email

VladimirMikulic commented Jan 3, 2020

SidharthBansal commented Jan 3, 2020 via email

SidharthBansal commented Jan 3, 2020 via email

VladimirMikulic commented Jan 3, 2020

SidharthBansal commented Jan 3, 2020 via email

SidharthBansal commented Jan 3, 2020 via email

sashadev-sky commented Jan 9, 2020

sashadev-sky commented Jan 9, 2020

VladimirMikulic commented Jan 9, 2020

sashadev-sky commented Jan 10, 2020

VladimirMikulic commented Jan 10, 2020 •

edited

Loading

ADD: setAngle() method #502

ADD: setAngle() method #502

Conversation

VladimirMikulic commented Dec 27, 2019 • edited Loading

jywarren commented Jan 2, 2020

VladimirMikulic commented Jan 2, 2020 • edited Loading

jywarren commented Jan 2, 2020

VladimirMikulic commented Jan 2, 2020 • edited Loading

jywarren commented Jan 2, 2020 via email

VladimirMikulic commented Jan 2, 2020 • edited Loading

jywarren commented Jan 3, 2020 via email

VladimirMikulic commented Jan 3, 2020

SidharthBansal commented Jan 3, 2020 via email

SidharthBansal commented Jan 3, 2020 via email

VladimirMikulic commented Jan 3, 2020

SidharthBansal commented Jan 3, 2020 via email

SidharthBansal commented Jan 3, 2020 via email

sashadev-sky commented Jan 9, 2020

sashadev-sky commented Jan 9, 2020

VladimirMikulic commented Jan 9, 2020

sashadev-sky commented Jan 10, 2020

VladimirMikulic commented Jan 10, 2020 • edited Loading

VladimirMikulic commented Dec 27, 2019 •

edited

Loading

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

VladimirMikulic commented Jan 2, 2020 •

edited

Loading

VladimirMikulic commented Jan 10, 2020 •

edited

Loading