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

Backportable keymap changes #1847

Merged
merged 9 commits into from Dec 18, 2018

Conversation

2 participants
@smashwilson
Copy link
Member

smashwilson commented Dec 17, 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.
  • Suggestion: You can use checklists to keep track of progress for the sections on metrics, tests, documentation, and user research.

Description of the Change

Restore MultiFilePatch navigation keybindings unadorned by cmd or ctrl that were removed by #1512. Specifically:

  • right and cmd-right or ctrl-right surface focus to the Git tab
  • backspace discards the selection
  • enter stages or unstages the selection
  • o jumps to file
  • / toggles between line and hunk selection mode

I've also separated the cmd and ctrl keybindings throughout the keymap by platform, guarding cmd bindings with .platform-darwin and ctrl alternatives with .platform-linux, .platform-win32. This opens up the ctrl- bindings on macOS in the future and prevents ambiguities in the binding that we render on buttons and tooltips.

Alternate Designs

Another alternative that we talked about was showing a notification the first time that you try to use one of the deprecated keystrokes (and letting the keystroke go through to the editor normally). I think we should do that as part of the diff editability work, personally.

Benefits

This keeps our keybinding changes from breaking users' muscle memory for another few releases.

Possible Drawbacks

If anyone on macOS learned the ctrl- keystrokes for some reason, those will be broken by this.

It also gives us a lot of redundancy in our keymap, which hurts maintainability. Hopefully this is just a temporary thing.

Applicable Issues

Related to #1820.

Metrics

Out of scope for the moment.

Tests

Our existing test suite should catch any regressions here.

Documentation

The flight manual already references unadorned keystrokes, which is a good reason to backport this. I've also already filed atom/flight-manual.atom.io#504 to update it to use the cmd or ctrl prefixes where appropriate, which we can merge while both work, so that new users learn the prefixed commands instead.

Release Notes

  • Restored unprefixed keyboard navigation commands to file patches in the GitHub package.

User Experience Research (Optional)

n/a

smashwilson added some commits Dec 17, 2018

@smashwilson smashwilson requested a review from atom/github-package Dec 17, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 17, 2018

Codecov Report

Merging #1847 into master will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1847      +/-   ##
==========================================
+ Coverage   90.85%   90.91%   +0.05%     
==========================================
  Files         195      195              
  Lines       10727    10727              
  Branches     1570     1570              
==========================================
+ Hits         9746     9752       +6     
+ Misses        981      975       -6
Impacted Files Coverage Δ
lib/controllers/editor-conflict-controller.js 95.95% <0%> (+1.01%) ⬆️
lib/atom/decoration.js 86.74% <0%> (+2.4%) ⬆️
lib/models/conflicts/side.js 96.87% <0%> (+4.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 755132b...a5fe3a0. Read the comment docs.

@annthurium
Copy link
Contributor

annthurium left a comment

thanks for fixing this, @smashwilson !

I agree that we should cherry pick this into stable to avoid thrash with key bindings which would frustrate our users.

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from In Progress 🔧 to QA Review 🔬 Dec 17, 2018

@smashwilson smashwilson merged commit 92065ee into master Dec 18, 2018

2 checks passed

codecov/patch Coverage not affected when comparing 755132b...a5fe3a0
Details
codecov/project 90.91% (+0.05%) compared to 755132b
Details

Stability Sprint : 20 November 2018 - 8 January 2019 : v0.24.0 automation moved this from QA Review 🔬 to Merged ☑️ Dec 18, 2018

@smashwilson smashwilson deleted the aw/keymap branch Dec 18, 2018

smashwilson added a commit that referenced this pull request Dec 18, 2018

Merge pull request #1847 from atom/aw/keymap
Backportable keymap changes

smashwilson added a commit that referenced this pull request Jan 4, 2019

Merge pull request #1847 from atom/aw/keymap
Backportable keymap changes

@smashwilson smashwilson referenced this pull request Jan 4, 2019

Closed

v0.23.2-0 QA Review #1883

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