Image-sequencer: contrast slider and apply buttons broken in main branch UI

[harshkhandeparkar]

The UI has broken again. :disappointed:
NOTE: THE LIVE DEMO IS WORKING

1. The form for selecting input doesn't work properly. The contrast option input in the above GIF is supposed to be a slider.
2. The apply button has broken itself again. :sob:
3. Too many errors in the console.

Where/How to reproduce

• https://beta.sequencer.publiclab.org
  The current beta site has this problem. (NOTE: The live demo is working fine)
  OR
  clone the repository and npm start

Fix

1. Go through the whole UI code. (t = 1hr)
2. Identify the error. (t = 2.5hr)
3. Try fixing the error. (t = 3hr)
4. Find out why the fix isn't working (t = 1hr)
5. Fix the fix. (t = 1hr)

Finally, after a month, the UI will break, again.

A much better, long-term solution

1. (this could be a big step): Use react or some other framework.:
   Frameworks make the management of UI super easy. Testing UI in react is a piece of cake (due to jest)

2. Add as many tests as possible: Extensively testing the UI code will make sure that nothing will break in the future.

3. Clean up the old code: Using some framework will compel us to clean up the old UI code which is a big mess at this point.

4. Isolate UI from the main library: This is the biggest step so far, but has to be done. HAS TO BE. Currently, the UI doesn't even work on some browsers. The files are built using grunt which is old (webpack is much better) We'll have to include the library as a dependency in the UI code and isolate the two completely. We could also create a new repo for it(as most frameworks need this)

5. We can use es6: The newest features in JS(es6) can be used without worrying about browser compatibility. This will make the code cleaner and better.

How do we do this?

Since we have to start somewhere, sometime(before the UI breaks completely), we'll have to test this.

1. Someone could try this in their own repo and if the results are good enough, we can create a repo in the organization.

1. Until the new UI is completely developed, we will have to stick to the old one.

2. Once the new UI is complete, we could slowly switch to it by initially having both and then stopping updates to the old one.

3. Frameworks like react create a complete static build which can be directly inserted in the examples/ folder for testing changes to the main library(local demo, npm start). If an issue is found in the UI, a PR can be opened in the UI repo and subsequently once the issue has been completely fixed, the fix can be integrated into the examples/.

4. This will allow for better maintenance of both the sections of this library. More than a single PR could be merged at once and everything will be manageable. The huge backlog of PRs will clear out.

---

Thank you!

Your help makes Public Lab better! We deeply appreciate your helping refine and improve this site.

To learn how to write really great issues, which increases the chances they'll be resolved, see:

https://publiclab.org/wiki/developers#Contributing+for+non-coders

[harshkhandeparkar]

@publiclab/is-reviewers @jywarren @Divy123 @aashna27 @tech4GT

[harshithpabbati]

Probably we can go with react and start remaking everything. Including the tests from the start. What do you say @jywarren @publiclab/is-reviewers ?

[rexagod]

I'd love to be a part of the remake! Very interesting! @jywarren, since we are considering a rebuild, could this be the refactor we were talking about (for the asynchronous aspects of IS)?

[harshkhandeparkar]

This issue is only talking about the UI part which is totally broken right
now.

On Fri, 30 Aug, 2019, 3:13 AM Pranshu Srivastava, [e-mail]
wrote:

I'd love to be a part of the remake! Very interesting! @jywarren
https://github.com/jywarren, since we are considering a rebuild, could
this be the refactor we were talking about (for the asynchronous aspects of
IS)?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/1238?email_source=notifications&email_token=AIJI5HYQKUR2TX2AB6V66D3QHA7JRA5CNFSM4ISEZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5P5JKY#issuecomment-526374059,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIJI5H3QMZYGGTOOW7HWNR3QHA7JRANCNFSM4ISEZJFA
.

[harshkhandeparkar]

I have a lot of experience in react. I have built full websites with it.
Backend, animations with react spring etc. I think will start the project
in my repo. I will let others know when the basic project is ready.

On Fri, 30 Aug, 2019, 8:41 AM M K, [e-mail] wrote:

This issue is only talking about the UI part which is totally broken right
now.

On Fri, 30 Aug, 2019, 3:13 AM Pranshu Srivastava, <
[email protected]> wrote:

I'd love to be a part of the remake! Very interesting! @jywarren
https://github.com/jywarren, since we are considering a rebuild, could
this be the refactor we were talking about (for the asynchronous aspects of
IS)?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/publiclab/image-sequencer/issues/1238?email_source=notifications&email_token=AIJI5HYQKUR2TX2AB6V66D3QHA7JRA5CNFSM4ISEZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5P5JKY#issuecomment-526374059,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AIJI5H3QMZYGGTOOW7HWNR3QHA7JRANCNFSM4ISEZJFA
.

[harshithpabbati]

@HarshKhandeparkar link me with the repo. I will be helping you :)

[harshkhandeparkar]

Sure

On Fri, 30 Aug, 2019, 10:43 AM Harshith Pabbati, [e-mail]
wrote:

@HarshKhandeparkar https://github.com/HarshKhandeparkar link me with
the repo. I will be helping you :)

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

[jywarren]

Hi all, I've changed the title. Thank you for catching this issue :heart:

However, I want to really emphasize that:

1. the breakage is in beta, not the main demo
2. recommendation 2) above is one which we have already committed to, but have not managed to follow up with -- UI testing can easily catch issues like this, and is not very hard to implement. I'd like to strongly push this option.
3. I am not at all opposed to react, to webpack, to es6, async additions, etc. -- they sound great. But I don't really see how they strongly relate to the UI breakage @HarshKhandeparkar found. Isn't the UI testing part the most critical point here? All the others sound great, but until we have UI tests that can catch this kind of issue, I would be hesitant to begin any other major projects. UI tests can also ensure that any new UI developed would not break existing features. Even if a new UI is in react, it will still fulfill basic UI tests such as "press this button and expect that to happen" -- you know? Starting a new UI project seems more likely to introduce additional bugs than to solve them, unless we start with tests, potentially!

Thank you all for your support and contribution to the library. I really want to refocus on UI testing as our number 1 priority before any other projects are acted upon. I definitely share Harsh's concern about the cycle of UI bugs we have faced, and I think UI testing is the most important thing we can do to catch them!

[harshkhandeparkar]

@jywarren I understand what you want to say. I think you didn't get why I said we should start with a new framework. This is so that we can make a new start. We could add tests along the way, from the start itself. A new UI would clean the code and protect it from future breakage.

[jywarren]

Thanks Harsh, i am just concerned that adding UI tests along the way is what we are already doing, but maybe not thoroughly enough, you know? What do you think of accelerating our UI testing work as our very top priority, as the thing that can most protect the code from breakage.

This can lay important groundwork for a /new/ UI project, but that kind of project may be really substantial and we'll need UI tests to safely demonstrate that it is not introducing new bugs itself. I'm really excited about the potential for new projects like that, but I'm very cautious about pursuing them /before/ investing our time in UI testing, because that feels like how we got here in the first place. That's why i am very 🎉 for your number 2 point above. What do you think of that as a plan?

[harshkhandeparkar]

Another benefit is that react takes care of the service worker and caches automatically. We just need to create a new clear cache btn.

[harshkhandeparkar]

@jywarren I actually said that the two things can be done simultaneously. We can still develop this UI and the new one will be developed in one of my repos (as discussed above). We will slowly start using the new UI once it is ready. Until then, no need to worry, the main repo will not be affected.

[harshkhandeparkar]

@rexagod @harshithpabbati the new repo is here https://github.com/HarshKhandeparkar/image-sequencer-ui. I will add you two as maintainers if you want.

[harshkhandeparkar]

I have added the basic scaffolding so far. Now we can build upon it.

@harshithpabbati @rexagod @jywarren @publiclab/is-reviewers

[harshkhandeparkar]

Issue regarding this in my repo: https://github.com/HarshKhandeparkar/image-sequencer-ui/issues/2
@jywarren @publiclab/is-reviewers

[jywarren]

Hi Harsh -- if you build a new UI, will the tests be applied here in this repository, and the new UI run against them? I left more notes in #1000 as well. Thank you!

[harshkhandeparkar]

I will make sure that the new UI has all the necessary tests. The UI can be
tested in its own repo if we split the repos or a new sub-project can be
made inside the examples/ which I won't recommend.

Instead, my plan is to use a separate UI repo as a github submodule in the
main repo.

On Mon, 2 Sep, 2019, 1:29 AM Jeffrey Warren, [e-mail]
wrote:

Hi Harsh -- if you build a new UI, will the tests be applied here in this
repository, and the new UI run against them? I left more notes in #1000
https://github.com/publiclab/image-sequencer/issues/1000 as well. Thank
you!

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

[harshkhandeparkar]

Lol, I found a new bug. This is crazy. If you click on the step collapse btn and if you have multiple steps, clicking on the load-image step collapses something totally different. After some time, all the collapse btns break.

[Divy123]

I also agree with @jywarren that right now, UI testing should be our top priority and all those changes related to new UI frameworks should be our next priority.

Secondly, I think using a UI framework like React does not guarantee that our UI doesn't break again.
Personally, I am also a react nanodegree graduate and the project, if goes with react is a great opportunity, but at this point of time, the main concern is getting each aspect of our library go smooth and I think we should use our full force of developers in that.

After that, I seriously believe that shifting to these can be a better option for us.

CC: @jywarren @blurry-x-face

[harshkhandeparkar]

Switching to React will make the code cleaner and manageable. If we are to
write tests now, we need to first fix the bugs(some of which are listed
here). The code is already very messy and fixing bugs is very difficult.

This is why I think we should already start developing a new UI
simultaneously so that we can switch to it asap.

On Tue, 3 Sep, 2019, 1:56 AM Slytherin, [e-mail] wrote:

I also agree with @jywarren https://github.com/jywarren that right now,
UI testing should be our top priority and all those changes related to new
UI frameworks should be our last priority.

Secondly, I think using a UI framework like React does not guarantee that
our UI doesn't break again.
Personally, I am also a react nanodegree graduate and the project, if goes
with react is a great opportunity, but at this point of time, the main
concern is getting each aspect of our library go smooth and I think we
should use our full force of developers in that.

After that, I seriously believe that shifting to these can be a better
option for us.

CC: @jywarren https://github.com/jywarren @blurry-x-face
https://github.com/blurry-x-face

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

[jywarren]

Thanks all for your thoughts. I have almost gotten HTML Jasmine UI tests running here, but I think that to truly test them in the browser we need to start tracking dist files again, although we can def. leave them .gitignored. https://github.com/publiclab/image-sequencer/pull/1242

or is there another way to run the tests? Like, could we add a grunt build step to the test suite workflow?

[harshkhandeparkar]

Can a grunt task run the tests?

[jywarren]

Yes, that's possible; you mean for better integration so we have like a
build, then test, task, and then a task which does those two?

On Thu, Sep 5, 2019 at 4:30 AM Harsh Khandeparkar [e-mail]
wrote:

Can a grunt task run the tests?

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

[harshkhandeparkar]

A grunt task which builds and then runs tests.

On Fri, 6 Sep, 2019, 3:26 AM Jeffrey Warren, [e-mail]
wrote:

Yes, that's possible; you mean for better integration so we have like a
build, then test, task, and then a task which does those two?

On Thu, Sep 5, 2019 at 4:30 AM Harsh Khandeparkar <
[email protected]>
wrote:

Can a grunt task run the tests?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<
https://github.com/publiclab/image-sequencer/issues/1238?email_source=notifications&email_token=AAAF6JYPZVLS2NNSVFURI7LQIC7O7A5CNFSM4ISEZJFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD56JNWQ#issuecomment-528258778
,
or mute the thread
<
https://github.com/notifications/unsubscribe-auth/AAAF6J5KCOEDZJFBBU753KLQIC7O7ANCNFSM4ISEZJFA

.

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

[harshkhandeparkar]

@jywarren @publiclab/is-reviewers please have a look at https://github.com/HarshKhandeparkar/image-sequencer-ui/pull/5

[jywarren]

I'm closing this now, in favor of the great progress we've made in Jest UI tests, and have copied relevant individual bug reports by @HarshKhandeparkar into https://github.com/publiclab/image-sequencer/issues/1369#issuecomment-657267387 for follow-up with specific UI tests. Thanks!

Specifically, the original reported issues were:

• contrast module input is supposed to be a slider, not a text input (solved)
• apply button doesn't work (addressed and tested in https://github.com/publiclab/image-sequencer/pull/1579)

Confirmed these work in GitPod on latest https://github.com/publiclab/image-sequencer/pull/1695 release candidate, and moving any remaining ones to #1369. Thanks!!!