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 close button to chain calibration #459

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

skiz
Copy link

@skiz skiz commented Nov 18, 2017

Add close button to calibration first step.

@skiz
Copy link
Author

skiz commented Nov 18, 2017

fixes #454

@blurfl
Copy link
Collaborator

blurfl commented Nov 18, 2017

This would be by way of cancelling the calibration, right? Would 'Cancel' be more appropriate than 'Close'?

@skiz
Copy link
Author

skiz commented Nov 18, 2017

@blurfl The only 'Cancel' in the app is in the save dialog, all popups use 'Close', so just following the existing design.

@blurfl
Copy link
Collaborator

blurfl commented Nov 19, 2017

Some of these dialogs put the machine into relative mode, let's make sure that exiting this way restores the machine to the condition it was in before it entered the page.

@BarbourSmith
Copy link
Member

I like it!

@blurfl is right that we have to be careful of the machine's state when exiting the calibration routine at weird points because once the "Begin" button is pressed things like relative mode, mm mode, and the chain lengths can be left in strange states. I feel good about the close button here in the first step because it is accessible before clicking the begin button. Using the back button to get back there then closing could be an issue, but that's already possible when fast forwarding to the end

@blurfl
Copy link
Collaborator

blurfl commented Nov 20, 2017

"...Using the back button to get back there then closing could be an issue, but that's already possible when fast forwarding to the end" This is probably how I get bitten, trying to use parts of the process. Do we need an issue to capture this?

@BarbourSmith BarbourSmith merged commit a4d1fcc into MaslowCNC:master Nov 20, 2017
@BarbourSmith
Copy link
Member

Yes! Great suggestion @blurfl

I will merge this as is and let's make a new issue for "it's not clear that exiting the calibration process part way through can cause trouble"

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.

3 participants