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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix so dialogs are canceled when pressing escape/closing them #1452

Merged
merged 1 commit into from May 8, 2018

Conversation

Projects
None yet
2 participants
@Ben3eeE
Member

Ben3eeE commented May 8, 2018

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

The label for canceling a dialog needs to be "Cancel" or "No" so that when canceling the dialog either by pressing escape or by closing the dialog cancels the dialog. Otherwise these actions default to the first item. This can be a bad default choice for the force push dialog which would force push when you close the dialog.

image

Alternate Designs

There is a cancelId option according to the electron documentation but this is not supported on Windows.

Benefits

  • Don't accidentally force push or merge with conflict markers when the user intended to cancel the dialog
  • Moves the cancel button to where it should be

Possible Drawbacks

  • Someone relies on pressing escape to execute a force push? 馃槄

Applicable Issues

Fixes #1368

@smashwilson

This comment has been minimized.

Member

smashwilson commented May 8, 2018

Someone relies on pressing escape to execute a force push? 馃槄

Oh no my workflow!

Haha, thanks @Ben3eeE 馃殌

@smashwilson smashwilson merged commit 383cb59 into master May 8, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the b3-escape-dialogs branch May 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment