-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: add project dashboard (#2303) #2320
Conversation
You can access the deployment of this PR at https://renku-ci-ui-2320.dev.renku.ch |
2505531
to
87d1167
Compare
59a58bb
to
4f72df9
Compare
4f72df9
to
e6133d9
Compare
e6133d9
to
4da97dc
Compare
4da97dc
to
c4dd73b
Compare
c4dd73b
to
d14b5eb
Compare
1f20402
to
aad9ce2
Compare
d14b5eb
to
c0efbe7
Compare
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 looks very good! 🚀
I noticed that some of the changes made for React 18 got lost in the rebase: those should be brought back. Otherwise, I have some questions about code in a small number of places and a few wording change suggestions. But, these are all minor things, and I think this I close to being ready to merge.
client/src/landing/Landing.test.js
Outdated
@@ -24,52 +24,60 @@ | |||
*/ | |||
|
|||
import React from "react"; | |||
import { createRoot } from "react-dom/client"; | |||
import ReactDOM from "react-dom"; |
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.
It looks like some of the changes that came from upgrading to react 18 were lost. The use of createRoot
should be brought back.
client/src/landing/Landing.test.js
Outdated
await act(async () => { | ||
root.render( | ||
ReactDOM.render( |
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 use createRoot
/ root.render
here.
client/src/landing/Landing.test.js
Outdated
await act(async () => { | ||
root.render( | ||
ReactDOM.render( |
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 use createRoot
/ root.render
here.
cy.url().should("be.equal", Cypress.config("baseUrl")); | ||
cy.get("[data-cy='username-home']").contains(`${username} @ Renku`); | ||
cy.get("[data-cy='dashboard-title']").contains(`Renku Dashboard - ${user.firstname} ${user.lastname}`); |
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.
cy.get("[data-cy='dashboard-title']").contains(`Renku Dashboard - ${user.firstname} ${user.lastname}`); | |
cy.get_cy("dashboard-title").contains(`Renku Dashboard - ${user.firstname} ${user.lastname}`); |
* useGetRecentlyVisitedProjects.ts | ||
* hook to fetch recent visited projects | ||
*/ | ||
function useGetRecentlyVisitedProjects(cantProjects: number) { |
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.
function useGetRecentlyVisitedProjects(cantProjects: number) { | |
function useGetRecentlyVisitedProjects(projectsCount: number) { |
cy.get_cy("inactive-kg-project-alert").should("exist"); | ||
}); | ||
|
||
it("user does not has own project but has visited projects", () => { |
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.
it("user does not has own project but has visited projects", () => { | |
it("user does not have own project but has visited projects", () => { |
}); | ||
}); | ||
|
||
it("user does has own projects and recently visited projects", () => { |
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.
it("user does has own projects and recently visited projects", () => { | |
it("user has own projects and recently visited projects", () => { |
return total === 0 ? | ||
<InfoAlert timeout={0}> | ||
<div data-cy="project-alert" className="mb-0" style={{ textAlign: "justify" }}> | ||
<h3><strong>You don’t have any project yet.</strong></h3> |
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.
I think it would be a bit better like this:
<h3><strong>You don’t have any project yet.</strong></h3> | |
<h3><strong>You do not have any projects yet.</strong></h3> |
else { | ||
projectsToShow = projects?.length > 0 ? | ||
<ProjectListRows projects={projects} gridDisplay={false} /> | ||
: <p className="rk-dashboard-section-header">You have no current project yet</p>; |
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.
I think current
is not necessary here
: <p className="rk-dashboard-section-header">You have no current project yet</p>; | |
: <p className="rk-dashboard-section-header">You do not have any recently-visited projects</p>; |
Thanks for the review @ciyer, yes I have brought back the changes in the tests and also apply all your suggestions in the last commit, all the text changes makes sense to me. |
f1b5a3a
to
92c66ba
Compare
aad9ce2
to
ad5f884
Compare
92c66ba
to
7869adb
Compare
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.
🎉 Ready to merge!
Tearing down the temporary RenkuLab deplyoment for this PR. |
* feat: add entity list view (#2302)(#2307) * feat: add project dashboard (#2303) (#2320) * feat: add dataset dashboard (#2304)(#2330) * feat: add session list view in dashboard (#2305)(#2333) * feat: add get logs, stop session and link to Dashboard (#2305) (#2350) * feat: modify the internal visibility icon, redirect to the dashboard when opening sessions page to authenticated users Fix #2286 Co-authored-by: Chandrasekhar Ramakrishnan <cramakri@ethz.ch>
Project dashboard PR.
Cases covered in this PR:
The user has projects to activate KG, must display the Warning Alert to activate the projects.
The user has no visited any project yet, it should show the button to explore other projects.
The user has visited some projects recently, it should show a maximum of 5 projects.
The user has own projects, it should not show information alert and show the View all my projects button.
Note: Here we do not cover the session view, in case the project has a running session the Connect button will be displayed instead of the play button, the session view will be implemented in #2306.
Closes #2303
/deploy renku=000-ui-dashboard #persist #cypress