Fixes #796

• [x] tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR -- or run tests locally with npm test
• [x] code is in uniquely-named feature branch and has no merge conflicts
• [x] PR is descriptively titled
• [x] ask @publiclab/is-reviewers for help, in a comment below

Codecov Report

Merging #1187 (a066a7e) into main (853e719) will decrease coverage by 1.41%. The diff coverage is 36.08%.

@@            Coverage Diff             @@
##             main    #1187      +/-   ##
==========================================
- Coverage   66.67%   65.26%   -1.42%     
==========================================
  Files         130      133       +3     
  Lines        2686     2752      +66     
  Branches      433      439       +6     
==========================================
+ Hits         1791     1796       +5     
- Misses        895      956      +61     

Impacted Files	Coverage Δ
src/Modules.js	100.00% <ø> (ø)
src/modules/EdgeDetect/Module.js	100.00% <ø> (ø)
src/modules/WebglDistort/Module.js	2.29% <0.00%> (ø)
src/modules/ColorHalftone/Module.js	3.63% <3.63%> (ø)
examples/lib/defaultHtmlStepUi.js	11.34% <11.11%> (-0.02%)	:arrow_down:
src/modules/ColorHalftone/index.js	100.00% <100.00%> (ø)
src/modules/Colorbar/Module.js	100.00% <100.00%> (ø)
src/modules/Crop/Module.js	88.88% <100.00%> (ø)
src/modules/EdgeDetect/EdgeUtils.js	86.81% <100.00%> (-0.15%)	:arrow_down:
src/modules/Resize/Module.js	100.00% <100.00%> (ø)
... and 4 more

@aashna27 why am I getting this error?

This error is unrelated! I got this sometime back as ImageSequencer doesn't load. But it isnt because of this pr.

Do you know how I can fix this?

Maybe try running it again! And use a different browser.

On Sun, Jul 28, 2019, 11:12 PM Harsh Khandeparkar [email protected] wrote:

Do you know how I can fix this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/image-sequencer/pull/1187?email_source=notifications&email_token=AGKQ7SH5ZMDHLJP3LWBOIE3QBXK6ZA5CNFSM4IHMMJX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD27DFNA#issuecomment-515781300, or mute the thread https://github.com/notifications/unsubscribe-auth/AGKQ7SFTPPRVNBIU43HDFDLQBXK6ZANCNFSM4IHMMJXQ .

Great work @aashna27 !! tada

thanks :smile:

gif to show the working

@HarshKhandeparkar @aashna27 please open an issue for that bug, thanks!

This looks great. Small conflict from the other PR by aashna.

Shall we put a cross hairs icon on the two fields which are to be populated by this new UI? Or a mouse pointer or finger pointer icon? Finger seems like a great idea! 👆🏽

And perhaps while the click listener is active, we should change the cursor itself? You know what though, let's do all that in a follow-up, why don't we?

So please open a follow-up issue to collect the refining ideas and as soon as this PR is clear, we can merge it!

Actually I looked and noticed there is no test for this - let's protect it with a UI test so it doesn't get broken in the future. Would you mind adding some simple tests? Thanks!

Actually as mentioned I have tried for UI tests and currently I believe @Divy123 is working on them. So once they are added we can do it for this one as well as other areas.

Indeed I remember, thanks! I'm just worried about the fragility of the new features we're adding, so perhaps you and @Divy123 should work to implement an initial version, even if imperfect, so as to get unblocked on this? Thanks, I appreciate it! I just want all this great new stuff to be properly protected.

On Sat, Aug 3, 2019, 12:28 PM aashna27 [email protected] wrote:

Actually as mentioned I have tried for UI tests and currently I believe @Divy123 https://github.com/Divy123 is working on them. So once they are added we can do it for this one as well as other areas.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/image-sequencer/pull/1187?email_source=notifications&email_token=AAAF6J6WPG6WBVNX3FH2R7DQCVFS5A5CNFSM4IHMMJX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3PK47Q#issuecomment-517910142, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6J4OTKBZWVUDA73HIGLQCVFS5ANCNFSM4IHMMJXQ .

Yeah it is really needed I do understand have communicated to divy and we ll work on something. Right now pushing my other tasks a little behind so that this can be done early.

Ok, awesome. Thanks for being flexible and for your great work!

On Sat, Aug 3, 2019, 12:50 PM aashna27 [email protected] wrote:

Yeah it is really needed I do understand have communicated to divy and we ll work on something. Right now pushing my other tasks a little behind so that this can be done early.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/publiclab/image-sequencer/pull/1187?email_source=notifications&email_token=AAAF6JYIYT2FZCZNFS4K5FLQCVIFXA5CNFSM4IHMMJX2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3PLKRQ#issuecomment-517911878, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAF6J4WWJ7LNQHFFTBFH7LQCVIFXANCNFSM4IHMMJXQ .

There is some issue with this. Width and height of crop cannot be selected

@aashna27

Noting i believe we have pretty good UI tests in Jest now! See these great examples: https://github.com/publiclab/image-sequencer/tree/main/test/ui-2/test

This will still need a UI test before merging. Thanks!