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

Add a 'Start a new sequence' button #589

Merged
merged 9 commits into from
Jan 7, 2019

Conversation

Mridul97
Copy link

@Mridul97 Mridul97 commented Jan 1, 2019

Fixes #496

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!

@jywarren
Copy link
Member

jywarren commented Jan 2, 2019

Hi, @Mridul97 is this almost the same as #581 by @aashna27 ? If so, perhaps you two could collab or review each others' solutions and submit a joint one?

Perhaps it should open in a new window in case people lose their work, too?

@Mridul97
Copy link
Author

Mridul97 commented Jan 3, 2019

Oh! Yes it seems quite similar, both are opening a new page with no steps but in this case we are opening it in a new tab. So people would not lose their work.

What would be better to click on the button "Start a new sequence" to open a new page or clicking on the header or both, depending on that maybe we can merge one or both PRs?

I think having a seperate button makes it more clear to the user. Thanks! 😄

@aashna27
Copy link

aashna27 commented Jan 3, 2019

@mridul but why add buttons unnecessarily when it can be done with the header, also most websites have this thing that you click on the logo or the name and it takes you to the home page. What say? 🤔

@jywarren
Copy link
Member

jywarren commented Jan 3, 2019 via email

@harshkhandeparkar
Copy link
Member

If the header restarts the sequence then there is a high chance that the user may click the header by mistake and loose the work. Instead i think the header should open a new page with a new sequence and the foooter should have a button which can clear the current sequence but with a conformation.

@aashna27
Copy link

aashna27 commented Jan 3, 2019

If the header restarts the sequence then there is a high chance that the user may click the header by mistake and loose the work. Instead i think the header should open a new page with a new sequence and the foooter should have a button which can clear the current sequence but with a conformation.

yess seems perfect !!

@jywarren
Copy link
Member

jywarren commented Jan 3, 2019 via email

@Mridul97
Copy link
Author

Mridul97 commented Jan 7, 2019

@jywarren Please have a look!

reset

@harshkhandeparkar
Copy link
Member

@Mridul97 I think the button should confirm if the user wants to reset the sequence. The user may click on that button instead of clicking on download png and may loose progress.

@Mridul97
Copy link
Author

Mridul97 commented Jan 7, 2019

@harshkhandeparkar Nice! I will add a confirmation option. Thanks!

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Jan 7, 2019 via email

@harshkhandeparkar
Copy link
Member

harshkhandeparkar commented Jan 7, 2019

@Mridul97 I have a few suggestions

you can shorten the if statement to:

if(r) window.location = "/";

No need to catch the e variable in the function.

Also can you use <button> instead of <input> for uniformity?

@Mridul97
Copy link
Author

Mridul97 commented Jan 7, 2019

Done! Here is the new output :

reset2

@@ -171,6 +171,7 @@ <h4 class="modal-title">Your gif is ready</h4>
<div class="panel-body">
<div style="text-align:center;">
<button class="btn btn-success btn-lg" id="download-btn" name="download">Download PNG</button>
<input type="button" id="resetButton" class="btn btn-success btn-lg" value="Reset Sequence"></a>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still a </a> remaining here @Mridul97.

@harshkhandeparkar
Copy link
Member

Looks awesome @Mridul97.

@jywarren
Copy link
Member

jywarren commented Jan 7, 2019

Awesome! Can we have it be a white button (btn-default, I think?) And say "clear all steps"? Since restart might sound like just rerunning everything.

Thanks!!

@Mridul97
Copy link
Author

Mridul97 commented Jan 7, 2019

@jywarren I have made the changes. Please have a look!

@jywarren jywarren merged commit 89345f3 into publiclab:main Jan 7, 2019
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