PRfD - List Pull Requests UI #8171

itaigilo · 2024-09-17T13:25:10Z

Closes #8173.

Change Description

Creating UI for Listing Pull Requests for a repo.
Including the new Pull Requests tab.

github-actions · 2024-09-17T13:33:42Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-09-17T13:34:04Z

E2E Test Results - Quickstart

itaigilo · 2024-09-17T14:07:05Z

Makefile

@@ -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


Helpful for UI dev.

We have npm run dev which achieves the same result

itaigilo · 2024-09-17T14:07:43Z

webui/src/lib/api/index.js

+        // return response.json();
+
+        // TODO: this is for development purposes only
+        console.log("list pulls", {repoId, state, prefix, after, amount})


As written - this is temp, until the API will be ready.

itaigilo · 2024-09-17T14:09:05Z

webui/src/lib/components/repository/changes.jsx

@@ -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">


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).

Can you explain please what was wrong?

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.

itaigilo · 2024-09-17T14:10:46Z

webui/src/lib/components/repository/tabs.jsx

@@ -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


Using the same icon as Compare. We'll need to change one of them.

Can we add a TODO here or open an issue

itaigilo · 2024-09-17T14:11:47Z

webui/src/lib/components/repository/tabs.jsx

@@ -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}>


Note - I've used pulls (and not pull-requests) pretty much everywhere.

itaigilo · 2024-09-17T14:13:02Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

This is based on the Tags page -
I changed mainly what I needed, and left the rest very similar.

N-o-Z

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

N-o-Z · 2024-09-17T15:34:20Z

Makefile

@@ -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


We have npm run dev which achieves the same result

N-o-Z · 2024-09-17T15:36:50Z

webui/src/lib/components/repository/changes.jsx

@@ -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">


Can you explain please what was wrong?

N-o-Z · 2024-09-17T15:37:04Z

webui/src/lib/components/repository/tabs.jsx

@@ -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


Suggested change

// TODO: this is temp, until PRfD will be ready

// TODO (gilo): this is temp, until PRfD will be ready

N-o-Z · 2024-09-17T15:37:33Z

webui/src/lib/components/repository/tabs.jsx

@@ -59,6 +60,11 @@ export const RepositoryNavTabs = ({ active }) => {
            <Link active={active === 'tags'} href={`/repositories/${repoId}/tags`} component={NavItem}>
                <TagIcon/> Tags
            </Link>
+            {showPulls &&


Please add the TODO here as well

N-o-Z · 2024-09-17T15:38:36Z

webui/src/lib/components/repository/tabs.jsx

@@ -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


Can we add a TODO here or open an issue

N-o-Z · 2024-09-17T15:40:00Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

+const PullWidget = ({repo, pull}) => {
+    const authorLink =
+        <Link href={{
+            // TODO: to where this link should lead?


Should lead to the user page in lakeFS IMO

N-o-Z · 2024-09-17T15:42:49Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

+    );
+};
+
+// TODO: is there a nicer place for this?


Suggested change

// TODO: is there a nicer place for this?

// TODO (gilo): is there a nicer place for this?

N-o-Z · 2024-09-17T16:04:51Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

+            {pull.author}
+        </Link>;
+    return (
+        <ListGroup.Item className="pull-row">


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

N-o-Z · 2024-09-17T16:09:21Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

+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?


state is a query param in GitHub

itaigilo

@N-o-Z Thanks for the review.

Your comments have been addressed, PTAL again.

itaigilo · 2024-09-17T17:36:56Z

webui/src/lib/components/repository/changes.jsx

@@ -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">


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.

N-o-Z

Awesome 🎉
I'm approving with 2 concerns:

Need to revert the flag!
The images still need adjusting - either in this PR or open a new issue for it. But we should have the same visuals as in compare:

N-o-Z · 2024-09-17T19:09:07Z

webui/src/lib/components/repository/tabs.jsx

-// 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;


Need to revert this to false 😃

itaigilo · 2024-09-18T05:46:40Z

@N-o-Z thanks for spotting the flag - reverted.

Regarding how the source/dest looks like:
I generally agree - I honestly don't have a clear understanding on how this should look from the PRD,
So I think that the best way to approach this will be to schedule a UI review once all the functionality is there.
I'll schedule such review, and will add an Issue for it.

itaigilo added 2 commits September 17, 2024 16:15

Add UI for List Pull Requests

42cb52d

Add watch build command

23ff775

itaigilo added exclude-changelog PR description should not be included in next release changelog minor-change Used for PRs that don't require issue attached labels Sep 17, 2024

itaigilo removed the minor-change Used for PRs that don't require issue attached label Sep 17, 2024

Update toolbar UI

7862d15

itaigilo requested review from N-o-Z and a team September 17, 2024 14:06

itaigilo marked this pull request as ready for review September 17, 2024 14:06

itaigilo commented Sep 17, 2024

View reviewed changes

N-o-Z requested changes Sep 17, 2024

View reviewed changes

Fix PR comments

512feb1

itaigilo commented Sep 17, 2024

View reviewed changes

itaigilo requested a review from N-o-Z September 17, 2024 17:37

N-o-Z approved these changes Sep 17, 2024

View reviewed changes

Hide feature

033a2a3

itaigilo merged commit feee276 into master Sep 18, 2024
38 checks passed

itaigilo deleted the feature/prfd-list-pull-requests-ui branch September 18, 2024 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRfD - List Pull Requests UI #8171

PRfD - List Pull Requests UI #8171

itaigilo commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024

github-actions bot commented Sep 17, 2024

itaigilo Sep 17, 2024

N-o-Z Sep 17, 2024

itaigilo Sep 17, 2024

itaigilo Sep 17, 2024

N-o-Z Sep 17, 2024

itaigilo Sep 17, 2024

itaigilo Sep 17, 2024

N-o-Z Sep 17, 2024

itaigilo Sep 17, 2024

itaigilo Sep 17, 2024

N-o-Z left a comment

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

N-o-Z Sep 17, 2024

itaigilo left a comment

itaigilo Sep 17, 2024

N-o-Z left a comment •

edited

Loading

N-o-Z Sep 17, 2024

itaigilo commented Sep 18, 2024

	// TODO: this is temp, until PRfD will be ready
	// TODO (gilo): this is temp, until PRfD will be ready

	// TODO: is there a nicer place for this?
	// TODO (gilo): is there a nicer place for this?

PRfD - List Pull Requests UI #8171

PRfD - List Pull Requests UI #8171

Conversation

itaigilo commented Sep 17, 2024 • edited Loading

Change Description

github-actions bot commented Sep 17, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Sep 17, 2024

E2E Test Results - Quickstart

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Sep 18, 2024

itaigilo commented Sep 17, 2024 •

edited

Loading

N-o-Z left a comment •

edited

Loading