Skip to content
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.

Sign up for GitHub

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

Jump to bottom

contrast slider and apply buttons broken in main branch UI #1238

Closed
harshkhandeparkar opened this issue Aug 29, 2019 · 26 comments
Closed

contrast slider and apply buttons broken in main branch UI #1238

harshkhandeparkar opened this issue Aug 29, 2019 · 26 comments
Labels
discussion help wanted important planning stability

Comments

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Aug 29, 2019 •
edited
Loading

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

ui-broken

  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. 😭
  3. Too many errors in the console.

Where/How to reproduce

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.

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

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

  4. 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/.

  5. 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
Copy link
Member Author

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

@harshithpabbati
Copy link

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
Copy link
Member

rexagod commented Aug 29, 2019

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
Copy link
Member Author

harshkhandeparkar commented Aug 30, 2019 via email

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Aug 30, 2019 via email

@harshithpabbati
Copy link

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

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Aug 30, 2019 via email

@jywarren jywarren changed the title IS is broken, again BETA IS is broken, again Aug 30, 2019
@jywarren
Copy link
Member

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

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
Copy link
Member Author

@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.

@harshkhandeparkar harshkhandeparkar changed the title BETA IS is broken, again Current IS UI is broken, again Aug 30, 2019
@harshkhandeparkar harshkhandeparkar changed the title Current IS UI is broken, again Current main branch IS UI is broken, again Aug 30, 2019
@jywarren
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link
Member Author

I have added the basic scaffolding so far. Now we can build upon it.
Screenshot from 2019-08-30 23-30-32

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

@harshkhandeparkar
Copy link
Member Author

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

@jywarren
Copy link
Member

jywarren commented Sep 1, 2019

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!

@jywarren jywarren mentioned this issue Sep 1, 2019
Closed
@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Sep 2, 2019 via email

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Sep 2, 2019 •
edited
Loading

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
Copy link
Member

Divy123 commented Sep 2, 2019 •
edited
Loading

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
Copy link
Member Author

harshkhandeparkar commented Sep 3, 2019 via email

@jywarren
Copy link
Member

jywarren commented Sep 4, 2019

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. #1242

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

@harshkhandeparkar
Copy link
Member Author

Can a grunt task run the tests?

@jywarren
Copy link
Member

jywarren commented Sep 5, 2019 via email

@harshkhandeparkar
Copy link
Member Author

harshkhandeparkar commented Sep 6, 2019 via email

@harshkhandeparkar
Copy link
Member Author

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

@harshkhandeparkar harshkhandeparkar mentioned this issue Oct 26, 2019
Closed
@vaibhavmatta vaibhavmatta added the gci-candidate Issue being considered for gci label Oct 28, 2019
@harshkhandeparkar harshkhandeparkar removed the gci-candidate Issue being considered for gci label Nov 2, 2019
@harshkhandeparkar harshkhandeparkar mentioned this issue Dec 9, 2019
Closed
5 tasks
@jywarren jywarren mentioned this issue Jul 12, 2020
Open
9 tasks
@jywarren
Copy link
Member

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 #1369 (comment) for follow-up with specific UI tests. Thanks!

Specifically, the original reported issues were:

Confirmed these work in GitPod on latest #1695 release candidate, and moving any remaining ones to #1369. Thanks!!!

image

@jywarren jywarren changed the title Current main branch IS UI is broken, again contrast slider and apply buttons broken in main branch UI Jul 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
discussion help wanted important planning stability
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jywarren @Divy123 @rexagod @harshkhandeparkar @harshithpabbati @vaibhavmatta