-
Notifications
You must be signed in to change notification settings - Fork 359
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
Conversation
Makefile
Outdated
@@ -342,6 +342,9 @@ $(UI_DIR)/node_modules: | |||
gen-ui: $(UI_DIR)/node_modules ## Build UI web app | |||
cd $(UI_DIR) && $(NPM) run build | |||
|
|||
gen-ui-watch: $(UI_DIR)/node_modules ## Build UI web app and watch for changes |
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
// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
As written - this is temp, until the API will be ready.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason, width: 100%
was added globally to float-start
, which is wrong.
Removed it, and added w-100
to all the places using it (and not overriding).
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.
Can you explain please what was wrong?
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.
float-start
is a utility class meant to, well, make elements float to the left.
By overriding it with other attributes, it makes it unusable when needed without the extras.
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.
@@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => { | |||
<Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}> | |||
<TagIcon/> Tags | |||
</Link> | |||
{showPulls && | |||
<Link active={active === 'pulls'} href={`/repositories/${repoId}/pulls`} component={NavItem}> | |||
<GitCompareIcon/> Pull Requests |
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.
Using the same icon as Compare
. We'll need to change one of them.
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.
Can we add a TODO here or open an issue
@@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => { | |||
<Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}> | |||
<TagIcon/> Tags | |||
</Link> | |||
{showPulls && | |||
<Link active={active === 'pulls'} href={`/repositories/${repoId}/pulls`} component={NavItem}> |
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.
Note - I've used pulls
(and not pull-requests
) pretty much everywhere.
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.
This is based on the Tags
page -
I changed mainly what I needed, and left the rest very similar.
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.
Really nice work! And great small and contained PR.
I'm blocking because I believe the tab design is not aligned with the overall lakeFS UI design and needs adjusting.
Since this is a major part of this PR I'm requesting changes
Makefile
Outdated
@@ -342,6 +342,9 @@ $(UI_DIR)/node_modules: | |||
gen-ui: $(UI_DIR)/node_modules ## Build UI web app | |||
cd $(UI_DIR) && $(NPM) run build | |||
|
|||
gen-ui-watch: $(UI_DIR)/node_modules ## Build UI web app and watch for changes |
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
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain please what was wrong?
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO: this is temp, until PRfD will be ready | |
// TODO (gilo): this is temp, until PRfD will be ready |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the TODO here as well
@@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => { | |||
<Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}> | |||
<TagIcon/> Tags | |||
</Link> | |||
{showPulls && | |||
<Link active={active === 'pulls'} href={`/repositories/${repoId}/pulls`} component={NavItem}> | |||
<GitCompareIcon/> Pull Requests |
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.
Can we add a TODO here or open an issue
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Should lead to the user page in lakeFS IMO
); | ||
}; | ||
|
||
// TODO: is there a nicer place for this? |
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.
// TODO: is there a nicer place for this? | |
// TODO (gilo): is there a nicer place for this? |
{pull.author} | ||
</Link>; | ||
return ( | ||
<ListGroup.Item className="pull-row"> |
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.
Look at the example of commits tab, it should look the same IMO.
Font size, alignment hyperlink marking and the visuals (images and colors) should have the same style.
This tab should look and feel like the other tabs
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 comment
The reason will be displayed to describe this comment to others. Learn more.
state is a query param in GitHub
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.
@N-o-Z Thanks for the review.
Your comments have been addressed, PTAL again.
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
float-start
is a utility class meant to, well, make elements float to the left.
By overriding it with other attributes, it makes it unusable when needed without the extras.
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.
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.
// TODO: this is temp, until PRfD will be ready | ||
const showPulls = false; | ||
// TODO (gilo): this is temp, until PRfD will be ready | ||
const showPulls = true; |
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.
Need to revert this to false
😃
@N-o-Z thanks for spotting the flag - reverted. Regarding how the source/dest looks like: |
Closes #8173.
Change Description
Creating UI for Listing Pull Requests for a repo.
Including the new
Pull Requests
tab.