-
Notifications
You must be signed in to change notification settings - Fork 355
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
PRfD - List Pull Requests UI #8171
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -567,6 +567,62 @@ class Tags { | |
|
||
} | ||
|
||
class Pulls { | ||
async list(repoId, state = "open", prefix = "", after = "", amount = DEFAULT_LISTING_AMOUNT) { | ||
// const query = qs({prefix, after, amount}); | ||
// const response = await apiRequest(`/repositories/${encodeURIComponent(repoId)}/pulls?` + query); | ||
// if (response.status !== 200) { | ||
// throw new Error(`could not list pulls: ${await extractError(response)}`); | ||
// } | ||
// return response.json(); | ||
|
||
// TODO: this is for development purposes only | ||
console.log("list pulls", {repoId, state, prefix, after, amount}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As written - this is temp, until the API will be ready. |
||
let results = [ | ||
{ | ||
"id": "test-pull-1", | ||
"title": "Test PR 1", | ||
"status": "open", | ||
"created_at": 1726575741, | ||
"author": "test-user-1", | ||
"description": "This is a test PR", | ||
"source_branch": "feature-branch-1", | ||
"destination_branch": "main" | ||
}, | ||
{ | ||
"id": "test-pull-2", | ||
"title": "Next Test PR 2", | ||
"status": "closed", | ||
"created_at": 1726402941, | ||
"author": "test-user-2", | ||
"description": "This is a another test PR", | ||
"source_branch": "feature-branch-2", | ||
"destination_branch": "main" | ||
}, | ||
{ | ||
"id": "test-pull-3", | ||
"title": "Another Test PR 3", | ||
"status": "open", | ||
"created_at": 1718454141, | ||
"author": "test-user-1", | ||
"description": "This is also a test PR", | ||
"source_branch": "feature-branch-3", | ||
"destination_branch": "feature-branch-1" | ||
} | ||
]; | ||
results = results.filter(pull => pull.status === state); | ||
return { | ||
"pagination": { | ||
"has_more": false, | ||
"max_per_page": 1000, | ||
"next_offset": "", | ||
"results": results.length | ||
}, | ||
"results": results | ||
} | ||
} | ||
} | ||
|
||
// uploadWithProgress uses good ol' XMLHttpRequest because progress indication in fetch() is | ||
// still not well supported across browsers (see https://stackoverflow.com/questions/35711724/upload-progress-indicators-for-fetch). | ||
export const uploadWithProgress = (url, file, method = 'POST', onProgress = null, additionalHeaders = null) => { | ||
|
@@ -652,7 +708,7 @@ class Objects { | |
if (response.status === 401) { | ||
return false; | ||
} | ||
|
||
// This is not one of the expected responses | ||
throw new Error(await extractError(response)); | ||
} | ||
|
@@ -1112,6 +1168,7 @@ class Import { | |
export const repositories = new Repositories(); | ||
export const branches = new Branches(); | ||
export const tags = new Tags(); | ||
export const pulls = new Pulls(); | ||
export const objects = new Objects(); | ||
export const commits = new Commits(); | ||
export const refs = new Refs(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,7 +153,7 @@ export const ChangesTreeContainer = ({results, delimiter, uriNavigator, | |
<div>{changesTreeMessage}</div> | ||
<Card> | ||
<Card.Header> | ||
<span className="float-start"> | ||
<span className="float-start w-100"> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For some reason, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain please what was wrong? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Basically, overriding bootstrap stuff should only be in case of something related to the theme of the app or something like this, things that are flavors of the app. Should almost never happen. |
||
{(delimiter !== "") && uriNavigator} | ||
</span> | ||
</Card.Header> | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -8,7 +8,8 @@ import {Link, NavItem} from "../nav"; | |||||
import {useRouter} from "../../hooks/router"; | ||||||
import {RefTypeBranch} from "../../../constants"; | ||||||
|
||||||
|
||||||
// TODO: this is temp, until PRfD will be ready | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
const showPulls = false; | ||||||
|
||||||
export const RepositoryNavTabs = ({ active }) => { | ||||||
const { reference } = useRefs(); | ||||||
|
@@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => { | |||||
<Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}> | ||||||
<TagIcon/> Tags | ||||||
</Link> | ||||||
{showPulls && | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add the TODO here as well |
||||||
<Link active={active === 'pulls'} href={`/repositories/${repoId}/pulls`} component={NavItem}> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note - I've used |
||||||
<GitCompareIcon/> Pull Requests | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using the same icon as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a TODO here or open an issue |
||||||
</Link> | ||||||
} | ||||||
<Link active={active === 'compare'} href={withRefAndCompareContext(`/repositories/${repoId}/compare`)} component={NavItem}> | ||||||
<GitCompareIcon/> Compare | ||||||
</Link> | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is based on the |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,159 @@ | ||||||
import React, {useEffect, useState} from "react"; | ||||||
import {useOutletContext} from "react-router-dom"; | ||||||
import Card from "react-bootstrap/Card"; | ||||||
import ListGroup from "react-bootstrap/ListGroup"; | ||||||
import Alert from "react-bootstrap/Alert"; | ||||||
import {Tab, Tabs} from "react-bootstrap"; | ||||||
import Button from "react-bootstrap/Button"; | ||||||
import TimeAgo from "react-timeago"; | ||||||
|
||||||
import {ActionGroup, AlertError, Loading, PrefixSearchWidget, RefreshButton} from "../../../../lib/components/controls"; | ||||||
import {pulls} from "../../../../lib/api"; | ||||||
import {useRefs} from "../../../../lib/hooks/repo"; | ||||||
import {useAPIWithPagination} from "../../../../lib/hooks/api"; | ||||||
import {Paginator} from "../../../../lib/components/pagination"; | ||||||
import {useRouter} from "../../../../lib/hooks/router"; | ||||||
import {RepoError} from "../error"; | ||||||
import {Link} from "../../../../lib/components/nav"; | ||||||
|
||||||
|
||||||
const PullWidget = ({repo, pull}) => { | ||||||
const authorLink = | ||||||
<Link href={{ | ||||||
// TODO: to where this link should lead? | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should lead to the user page in lakeFS IMO |
||||||
pathname: '/repositories/:repoId/user/:userId', | ||||||
params: {repoId: repo.id, userId: pull.author} | ||||||
}}> | ||||||
{pull.author} | ||||||
</Link>; | ||||||
return ( | ||||||
<ListGroup.Item className="pull-row"> | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Look at the example of commits tab, it should look the same IMO. |
||||||
<div className="clearfix"> | ||||||
<div className="float-start pt-1 pb-2"> | ||||||
<Link className="pull-title fs-4" | ||||||
href={{ | ||||||
pathname: '/repositories/:repoId/pulls/:pullId', | ||||||
params: {repoId: repo.id, pullId: pull.id}, | ||||||
}} | ||||||
> | ||||||
{pull.title} | ||||||
</Link> | ||||||
<div className="pull-info mt-1"> | ||||||
Opened <TimeAgo date={new Date(pull.created_at * 1000)}/> ago by {authorLink} | ||||||
</div> | ||||||
</div> | ||||||
<div className="pull-branches mt-3 float-end"> | ||||||
<Button variant="secondary" size="sm" disabled={true}>{pull.source_branch}</Button> | ||||||
<span className="m-2">⇨</span> | ||||||
<Button variant="secondary" size="sm" disabled={true}>{pull.destination_branch}</Button> | ||||||
</div> | ||||||
</div> | ||||||
</ListGroup.Item> | ||||||
); | ||||||
}; | ||||||
|
||||||
// TODO: is there a nicer place for this? | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
const PullStatus = { | ||||||
open: "open", | ||||||
closed: "closed", | ||||||
merged: "merged", | ||||||
} | ||||||
|
||||||
const PullsList = ({repo, after, prefix, onPaginate}) => { | ||||||
const router = useRouter() | ||||||
const [refresh, setRefresh] = useState(true); | ||||||
// TODO: pullState should be persistent in the url and saved as a url param? | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. state is a query param in GitHub |
||||||
const [pullsState, setPullsState] = useState(PullStatus.open); | ||||||
const {results, error, loading, nextPage} = useAPIWithPagination(async () => { | ||||||
return pulls.list(repo.id, pullsState, prefix, after); | ||||||
}, [repo.id, pullsState, prefix, refresh, after]); | ||||||
|
||||||
const doRefresh = () => setRefresh(true); | ||||||
|
||||||
let content; | ||||||
|
||||||
if (loading) content = <Loading/>; | ||||||
else if (error) content = <AlertError error={error}/>; | ||||||
else content = (results && !!results.length ? | ||||||
<> | ||||||
<Card> | ||||||
<ListGroup variant="flush"> | ||||||
{results.map(pull => ( | ||||||
<PullWidget key={pull.id} repo={repo} pull={pull}/> | ||||||
))} | ||||||
</ListGroup> | ||||||
</Card> | ||||||
<Paginator onPaginate={onPaginate} nextPage={nextPage} after={after}/> | ||||||
</> : <Alert variant="info">There aren't any pull requests yet.</Alert> | ||||||
) | ||||||
|
||||||
return ( | ||||||
<div className="mb-5"> | ||||||
<div className="position-relative clearfix"> | ||||||
<div className=""> | ||||||
<Tabs | ||||||
defaultActiveKey={pullsState} | ||||||
id="pulls-tabs" | ||||||
onSelect={key => setPullsState(key)} | ||||||
className="mb-3 pt-2" | ||||||
> | ||||||
<Tab eventKey={PullStatus.open} title="Open"/> | ||||||
<Tab eventKey={PullStatus.closed} title="Closed"/> | ||||||
</Tabs> | ||||||
</div> | ||||||
<ActionGroup orientation="right" className="position-absolute top-0 end-0 pb-2"> | ||||||
<PrefixSearchWidget | ||||||
defaultValue={router.query.prefix} | ||||||
text="Find Pull Request" | ||||||
onFilter={prefix => router.push({ | ||||||
pathname: '/repositories/:repoId/pulls', | ||||||
params: {repoId: repo.id}, | ||||||
query: {prefix} | ||||||
})}/> | ||||||
|
||||||
<RefreshButton onClick={doRefresh}/> | ||||||
<Button variant="success">Create Pull Request</Button> | ||||||
</ActionGroup> | ||||||
</div> | ||||||
{content} | ||||||
</div> | ||||||
); | ||||||
}; | ||||||
|
||||||
|
||||||
const PullsContainer = () => { | ||||||
const router = useRouter() | ||||||
const {repo, loading, error} = useRefs(); | ||||||
const {after} = router.query; | ||||||
const routerPfx = router.query.prefix || ""; | ||||||
|
||||||
if (loading) return <Loading/>; | ||||||
if (error) return <RepoError error={error}/>; | ||||||
|
||||||
return ( | ||||||
<PullsList | ||||||
repo={repo} | ||||||
after={after || ""} | ||||||
prefix={routerPfx} | ||||||
onPaginate={after => { | ||||||
const query = {after}; | ||||||
if (router.query.prefix) { | ||||||
query.prefix = router.query.prefix; | ||||||
} | ||||||
router.push({ | ||||||
pathname: '/repositories/:repoId/pulls', | ||||||
params: {repoId: repo.id}, | ||||||
query | ||||||
}); | ||||||
}}/> | ||||||
); | ||||||
}; | ||||||
|
||||||
|
||||||
const RepositoryPullsPage = () => { | ||||||
const [setActivePage] = useOutletContext(); | ||||||
useEffect(() => setActivePage("pulls"), [setActivePage]); | ||||||
return <PullsContainer/>; | ||||||
} | ||||||
|
||||||
export default RepositoryPullsPage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful for UI dev.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have
npm run dev
which achieves the same result