-
Notifications
You must be signed in to change notification settings - Fork 210
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
[FIXED]Draw rectangle's rectangle shinking down with every frame in gif's #1535
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1535 +/- ##
=======================================
Coverage 66.61% 66.61%
=======================================
Files 130 130
Lines 2672 2672
Branches 430 430
=======================================
Hits 1780 1780
Misses 892 892 |
@publiclab/is-reviewers |
Why is this open as a PR? Shouldn't you open a issue instead? Or is this already solved, In that case please share the working Gif! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you protect this with a test? #1530 includes test for draw rectangle but isn't it not working expectedly?
It was an issue only in gif's. A test do exist in my GIF's Test PR..... |
How is test for draw rectangle passing on that other PR? |
We can get this merged soon and then you can correct the test on this accordingly! |
I remember I had fixed this nonsense when I added GIFs for the first time.
Maybe dome other PR broke it. Anyway. All you have to do to fix this is to
not edit the initial `options.size` make the options immutable and just use
the value. Pretty simple.
…On Sun, 19 Jan, 2020, 9:38 AM Rishabh Shukla, ***@***.***> wrote:
How is test for draw rectangle passing on that other PR?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
<#1535?email_source=notifications&email_token=AIJI5H4ETZIA6XZ6T4LIJPLQ6PG4NA5CNFSM4KIRUI42YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJKIP4Y#issuecomment-575965171>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AIJI5H5OLVTWAKSSFYT6P33Q6PG4NANCNFSM4KIRUI4Q>
.
|
@blurry-x-face the test written in #1530 is correct. That is why it's failing... Once this PR gets merged, it will pass itself. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect!
@publiclab/is-reviewers @jywarren @harshkhandeparkar SOMETHING IS BROKE, npm test is failing in like 30 tests. |
found it, #1533 changed the PixelManipulation.js, that's the issue....Maybe consider reverting back the PR.... |
Ah, thanks for being careful here, is this resolved?
|
Nope, The modules test are still failing.... And travis is not able to detect. |
Great, sorry for slow review here, just waiting for last checks and will merge this then look at #1530! |
Perfect! Thank you! |
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
npm test
@publiclab/is-reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Please make sure to get at least two reviews before asking for merging the PR as that would make the PR more reliable on our part
Thanks!