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.

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

Added insert step button at every step #545

Merged
merged 14 commits into from
Dec 17, 2018
Merged

Added insert step button at every step #545

merged 14 commits into from
Dec 17, 2018

Conversation

Divy123
Copy link
Member

@Divy123 Divy123 commented Dec 15, 2018

Fixes #466

Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!

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

We're happy to help you get this ready -- don't be afraid to ask for help, and don't be discouraged if your tests fail at first!

If 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

Thanks!

@Divy123
Copy link
Member Author

Divy123 commented Dec 15, 2018

@publiclab/reviewers please review.

@harshkhandeparkar
Copy link
Member

@Divy123 please provide a gif/screenshot for the PR.

@harshkhandeparkar
Copy link
Member

Also you should probably change the arrow functions to normal ones as it is not supported by all browsers

@harshkhandeparkar
Copy link
Member

Also you can add a fontawesome icon to the btn if you want.

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

@Divy123 Were you able to figure out the correct index to run the sequence from?

@Divy123
Copy link
Member Author

Divy123 commented Dec 16, 2018

@tech4GT actually I tried to run it from the current index and some other related values but then I realized that run was only responsible for producing output and the UI that was being added was due to insertSteps() which inserted it correctly in the steps array so I reloaded the page after every insert step which correctly displays all steps in correct order. Hope that is correct.

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

Hmm, I think we should not have to reload the page for it to work, hold on I'll take a look.

@Divy123
Copy link
Member Author

Divy123 commented Dec 16, 2018

Sure @tech4GT please take a look.

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

Okay Divy, I am getting a couple of errors here, I think Image not loading is because of some errors in defaultStepUI file, which is preventing the javascript on the page to run any further. Let me fix this up a little. Thanks.

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

@jywarren Can you help out a little here, I am trying to isolate the code which appends the step in the div, while adding steps and which removes the step from the html when we we remove a step.

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

Ok, I have isolated the problem here, the defaultStep UI always appends the new step at the end, we need to fix it to append at the right position, then it'll work perfectly!
cc @jywarren @Divy123

@ghost ghost assigned tech4GT Dec 16, 2018
@ghost ghost added the in-progress label Dec 16, 2018
@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

@Divy123 Pull these changes and try it out, I think this should work now! 😄

@Divy123
Copy link
Member Author

Divy123 commented Dec 16, 2018

@tech4GT yes its working perfectly. Thanks a lot 😄 . The Travis is failing at one test for node v6 at npm install. Can you please guide on that?

@tech4GT
Copy link
Member

tech4GT commented Dec 16, 2018

Divy try opening and closing the pull request.

@Divy123 Divy123 closed this Dec 16, 2018
@ghost ghost removed the in-progress label Dec 16, 2018
@Divy123 Divy123 reopened this Dec 16, 2018
@Divy123
Copy link
Member Author

Divy123 commented Dec 16, 2018

@tech4GT all tests passed. I think it is ready now. Thanks a lot for your help 😄 .

@tech4GT
Copy link
Member

tech4GT commented Dec 17, 2018

Divy, I think this needs a bit more work, I don't think this is working correctly for sequences and I noticed that saving sequences is also not working out correctly. I'll push the fixes in and then we can merge.

@ghost ghost added the in-progress label Dec 17, 2018
@tech4GT
Copy link
Member

tech4GT commented Dec 17, 2018

@Divy123 I think we can do one more thing here, disable the add button for the last step, can you do that? Thanks.

@tech4GT
Copy link
Member

tech4GT commented Dec 17, 2018

And sorry to pile in all this work in this pull request, Actually I have not really been coding a lot on this project lately and I saw some small errors here and there so I fixed them.

@Divy123
Copy link
Member Author

Divy123 commented Dec 17, 2018

Sure @tech4GT . I am upon it and thanks for your help.

@Divy123
Copy link
Member Author

Divy123 commented Dec 17, 2018

@tech4GT please have a look.

@tech4GT
Copy link
Member

tech4GT commented Dec 17, 2018

Great! I think we are good to go here! 🎉

@tech4GT tech4GT merged commit 23f4851 into publiclab:main Dec 17, 2018
@ghost ghost removed the in-progress label Dec 17, 2018
@Divy123
Copy link
Member Author

Divy123 commented Dec 17, 2018

Thanks @tech4GT .

@jywarren
Copy link
Member

Fantastic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants