Commit with keyboard action #376
Conversation
lib/controllers/git-controller.js
Outdated
| props.commandRegistry.add('atom-workspace', { | ||
| 'github:toggle-git-panel': this.toggleGitPanel, | ||
| 'github:toggle-git-panel-focus': this.toggleGitPanelFocus, | ||
| 'github:commit': this.ensureGitPanel, |
There was a problem hiding this comment.
Listening for github:commit both here and in the CommitView feels wrong to me. It also makes it awkward to recognize when the git panel was just opened and thus should never actually commit.
I'm not certain what the best solution is, here. I'm thinking that it would be best to keep this as the only github:commit handler and have it drive behavior within the child components. I know React and/or Enzyme (with a shallow render) make it awkward to access child components directly... @BinaryMuse, is there a more React-y way to trigger behavior in children?
There was a problem hiding this comment.
The main obstacle to lifting all of the commit logic up to GitController is that the commit message itself is stored within the TextEditor way down in CommitView.
Resolve a Promise as `true` if it had been previously hidden or `false` if it was already shown.
| message = this.props.mergeMessage; | ||
| const message = this.getCommitMessage(); | ||
| if (this.props.onChangeMessage) { | ||
| this.props.onChangeMessage(message); |
There was a problem hiding this comment.
Hurf, I thought I reverted this.
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
This is working, but some of the internals still feel odd: it's weird to me that ensureGitPanel is passed down into the component hierarchy while prepareToCommit is passed up.
| message = this.props.mergeMessage; | ||
| } | ||
| return message; | ||
| } |
There was a problem hiding this comment.
Ambient refactoring left over from a previous iteration on the implementation here. This bit looks cleaner to me so I left it.
|
|
||
| // Ensure that the Git panel is visible. Returns a Promise that resolves to `true` if the panel was initially | ||
| // hidden or `false` if it was already shown. | ||
| ensureGitPanel() { |
There was a problem hiding this comment.
I picked this name for consistency with the existing toggleGitPanel() but it reads oddly to me. Maybe they should be toggleGitPanelVisibility() and ensureGitPanelVisibility(), or toggleGitPanelActive() and ensureGitPanelActive()?
| }); | ||
| } | ||
|
|
||
| return Promise.resolve(false); |
There was a problem hiding this comment.
The Promise is needed here, because this is called from prepareToCommit() which needs its return value.
|
|
||
| async prepareToCommit() { | ||
| if (await this.props.ensureGitPanel()) { | ||
| this.focus(); |
There was a problem hiding this comment.
I think this .focus() call is redundant; CommitView.commit() will focus the commit editor anyway.
| this.props = props; | ||
|
|
||
| this.commit = this.commit.bind(this); | ||
| this.abortMerge = this.abortMerge.bind(this); |
There was a problem hiding this comment.
🔥 duplicate code!
Repurpose the
github:commitcommand to:Trigger the
github:commitcommand with(ctrl/cmd)-enteranywhere within the git panel.Remaining Work
github:commitis dispatched, open the panel and focus the CommitView, but don't actually commit, even if all of the other state is correct. It'd be weird to commit without being able to see what you're doing first.Fixes #174.