Show login window in GH panel #474
Conversation
lib/git-shell-out-strategy.js
Outdated
| } | ||
|
|
||
| async getRemotes() { | ||
| const output = await this.exec(['config', '--local', '--get-regexp', '^remote..*.url$']); |
There was a problem hiding this comment.
@kuychaco I couldn't think of a better way to do this. Any thoughts?
There was a problem hiding this comment.
Also, I wonder if we should specifically look at pushUrl here too, in case PRs are opened against pushUrl's repo and not url's
smashwilson
left a comment
smashwilson
left a comment
There was a problem hiding this comment.
👍 Various tiny comments on superficial stuff.
| import Resizer from '../views/resizer'; | ||
| import Tabs from '../views/tabs'; | ||
| import Commands, {Command} from '../views/commands'; | ||
| import GithubController from './github-controller'; |
There was a problem hiding this comment.
Structurally, it's a little weird to me that we have a GitController that contains a GitPanelController and a GithubController, especially because the UI consists of a single panel that our registered commands call a "git panel". It'd be nice to have some symmetry between the classnames on each side and between the UI language and the source.
Oh, also, shouldn't the capitalization be GitHub for Branding Reasons ™️?
Maybe instead we could have:
GitPanelControllerGitControllerGitHubController
Or:
GitControllerGitTabControllerGitHubTabController
(We could also defer any renaming until after this particular PR ships; I don't want to hold it up for us to decide on that bikeshed color 😉)
There was a problem hiding this comment.
I agree, the naming of these classes needs work. I also agree that we'll do it later ;)
lib/controllers/github-controller.js
Outdated
| <p> | ||
| This repository has multiple remotes hosted at GitHub.com. | ||
| Select a remote to see pull requests associated | ||
| with this branch ({currentBranch}). |
There was a problem hiding this comment.
Maybe something like with the <strong>{currentBranch}</strong> branch to save some parentheticals?
(I always wildly overuse parentheticals myself.)
| {remotes.map(remote => ( | ||
| <li key={remote.name}> | ||
| <a href="#" onClick={e => selectRemote(e, remote)}> | ||
| {remote.name} ({remote.info.owner}/{remote.info.name}) |
There was a problem hiding this comment.
Ooh nice ✨ This will save me so many git remote -v calls 😁
lib/controllers/github-controller.js
Outdated
|
|
||
| render() { | ||
| if (!this.props.repository || !this.props.remotes) { | ||
| return <div />; |
There was a problem hiding this comment.
React thing - is this better than returning null?
| let remote = this.props.remotes.find(r => r.name === this.props.selectedRemote); | ||
| let remotesAvailable = false; | ||
| if (!remote && this.props.remotes.length === 1) { | ||
| remote = this.props.remotes[0]; |
There was a problem hiding this comment.
💯 for defaulting to a single remote with a prompt.
| render() { | ||
| return ( | ||
| <div className="github-RemotePrController"> | ||
| {this.props.token && <span>you gots a token! {this.props.token}</span>} |
lib/decorators/observe-model.js
Outdated
| * // and should return the model to watch; defaults to `props.model` | ||
| * getModel: props => props.repository, | ||
| * // fetchData takes the model instance and the props passed | ||
| * // to the outer component and should an object (or promise |
There was a problem hiding this comment.
Missing the word "return" here I think ☝️
There was a problem hiding this comment.
Reminds me of the time I accidentally the whole thing
lib/git-shell-out-strategy.js
Outdated
| async getRemotes() { | ||
| let output; | ||
| try { | ||
| output = await this.exec(['config', '--local', '--get-regexp', '^remote..*.url$']); |
There was a problem hiding this comment.
Any way this could be a this.getConfig() call?
There was a problem hiding this comment.
I thought about this, but it would complicate calling getConfig quite a bit so I decided to forego on that effort. I could be convinced.
There was a problem hiding this comment.
I lied, this is a great idea 😅
| } catch (_e) { | ||
| // Nothing | ||
| } | ||
| } |
There was a problem hiding this comment.
Oh wow, this'll be great to have.
There was a problem hiding this comment.
The only downside is to make the React devtools work you have to remove the CSP meta tag in static/index.html in your local Atom build (assuming dev mode).
Implements the following flows: