Update repo flow to use popups #4979
Conversation
| font-family: system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen, Ubuntu, Cantarell, Fira Sans, | ||
| Droid Sans, Helvetica Neue, sans-serif; |
There was a problem hiding this comment.
not sure if you wanted this to match the main app font, but we're using Open Sans currently
| font-family: system-ui, -apple-system, BlinkMacSystemFont, Segoe UI, Roboto, Oxygen, Ubuntu, Cantarell, Fira Sans, | |
| Droid Sans, Helvetica Neue, sans-serif; | |
| font-family: "Open Sans", sans-serif; |
There was a problem hiding this comment.
I'd like to try using this system-ui stack that we use for our marketing page. I messed around with making this the default for our whole app UI as well, but wanted to wait a bit and test it out before taking the leap.
enterprise/app/repo/repo.tsx
Outdated
| @@ -118,16 +131,57 @@ export default class RepoComponent extends React.Component<RepoComponentProps, R | |||
| this.fetchSecrets(); | |||
| } | |||
|
|
|||
| async loginAndLinkGithub() { | |||
| if (!this.props.user) { | |||
| await this.loginToGithub(); | |||
There was a problem hiding this comment.
if loginToGithub fails then it will continue to linkGithubAccount, because loginToGithub does catch on the promise. Is that OK?
There was a problem hiding this comment.
Ah, I didn't quite know how catch interacted with await. I've updated this to remove the catches and added a single try/catch to protect the whole thing.
Side note, I think async/await is provides a such nice syntax for flattening deeply nested serial promises - but the try catch syntax makes it look so ugly :(
enterprise/app/repo/repo.tsx
Outdated
| isGCPDeploy && !this.state.secretsResponse?.secret.map((s) => s.name).includes(gcpRefreshTokenKey); | ||
|
|
||
| if (isGCPDeploy && needsGCPLink) { | ||
| await this.linkGoogleCloud() |
There was a problem hiding this comment.
if there's an error then it will continue due to the catch - is that OK here too?
There was a problem hiding this comment.
Similar to above, I removed that catch and extended the scope of the try/catch below to cover this.
This is still a bit of a work in progress, but this state is definitely better than what's currently live in dev.
This PR does a few things, happy to break it up if it's a pain to review:
Screenshot:
