feat(review-board): add review board support #98
Conversation
KattMingMing
force-pushed
the
rb-support
branch
2 times, most recently
from
bd25f06 to
f8003b6
Compare
KattMingMing
force-pushed
the
rb-support
branch
from
f8003b6 to
590f930
Compare
| import { ReviewBoardRepository } from '../util' | ||
|
|
||
| function getRepository(repoID: number): Promise<ReviewBoardRepository> { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
You shouldn't use the Promise constructor here, fetch already returns a Promise.
| getCodeElementFromLineNumber: () => null, | ||
| getLineNumberFromCodeElement, | ||
| getDiffCodePart, | ||
| } |
There was a problem hiding this comment.
I would propose to not have these in a util file, but in a file dom_functions like we do for GitHub.
| } | ||
| const { repository, reviewRequest } = reviewBoardState | ||
| getRepositoryFromReviewBoardAPI(repository.id).subscribe(rbRepository => { | ||
| resolveRepoPathFromName(rbRepository).subscribe( |
There was a problem hiding this comment.
Don't nest subscribe, instead use switchMap
| repository, | ||
| reviewURL, | ||
| state, | ||
| } |
There was a problem hiding this comment.
Is your goal here just to clone the object? Why not use { ...page.reviewRequest.attributes }?
| } | ||
|
|
||
| document.dispatchEvent( | ||
| new CustomEvent('REVIEW_BOARD_LOADED', { |
| detail: { | ||
| repository: page.reviewRequest!.attributes.repository, | ||
| reviewRequest, | ||
| } as ReviewBoardState, |
There was a problem hiding this comment.
Casting hides errors, it's better to declare it as a variable first.
| // Reviewboard has a global RB function on that DOM that we rely on to get ReviewBoard state. | ||
| RB: { | ||
| PageManager: { | ||
| getPage: () => any |
There was a problem hiding this comment.
Can this be typed instead of any? It will be hard for people to maintain if they don't know what this returns
|
Here's how to move this PR to https://github.com/sourcegraph/sourcegraph/tree/master/packages/browser-extensions |
This PR changes:
Testing plan:
I have tested on: