-
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
[ShapeUp] Renku V2 Project Page #3108
Conversation
You can access the deployment of this PR at https://renku-ci-ui-3108.dev.renku.ch |
1f74a2a
to
4580ba2
Compare
a2871ef
to
3b6cae9
Compare
Co-authored-by: Flora Thiebaut <flora.thiebaut@sdsc.ethz.ch>
Add project page header, menu and project info --------- Co-authored-by: Flora Thiebaut <flora.thiebaut@sdsc.ethz.ch>
…options to add/edit/delete session launcher and add/delete code repositories
…vironments available, and fix the copyright year for new components.
…. Remove status and date for code repositories
3b6cae9
to
5bb8cd9
Compare
Merge pull request #3133 from SwissDataScienceCenter/leafty/update-project-page-merge
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 mostly ready 👏 , some comments below.
The [design] items are non-blocking but need to be raised with design and fixed shortly.
client/.eslintrc.json
Outdated
@@ -212,6 +213,7 @@ | |||
"readme", | |||
"readonly", | |||
"Ravey", | |||
"rcloud", |
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.
"rcloud", |
import { | ||
default as buttonStyles, | ||
default as styles, | ||
} from "./Buttons.module.scss"; |
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.
There is no reason to import this twice:
import { | |
default as buttonStyles, | |
default as styles, | |
} from "./Buttons.module.scss"; | |
import styles from "./Buttons.module.scss"; |
const classesNames = props.className?.length | ||
? props.className?.split(" ") | ||
: []; |
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 use cx()
from classnames
.
const options = props.children ? ( | ||
<> | ||
<DropdownToggle | ||
data-cy="more-menu" | ||
className={`${props.className} ${classes}`} | ||
className={cx(...classesNames, classes, "rounded-end-pill")} |
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.
className={cx(...classesNames, classes, "rounded-end-pill")} | |
className={cx(props.classesName, classes, "rounded-end-pill")} |
@@ -98,7 +111,7 @@ function ButtonWithMenu(props: ButtonWithMenuProps) { | |||
return ( | |||
<ButtonDropdown | |||
id={props.id} | |||
className={`${props.className} btn-with-menu`} | |||
className={cx("btn-with-menu", classesNames)} |
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.
className={cx("btn-with-menu", classesNames)} | |
className={cx(props.className, "btn-with-menu")} |
client/src/styles/assets/env.svg
Outdated
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.
[design]: Should use viewbox of 32x32.
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.
[design]: Should use viewbox of 32x32.
const badge = | ||
state === "running" && defaultImage ? ( | ||
<SessionBadge className="border border-warning bg-warning-subtle"> | ||
<ExclamationCircleFill className="text-warning" size={16} /> |
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 all bootstrap icons, use class "bi"
to align them with the surrounding text. Also, you can handle the margin here:
<ExclamationCircleFill className="text-warning" size={16} /> | |
<ExclamationCircleFill className={cx("bi", "me-1", "text-warning")} size={16} /> |
) : state === "starting" ? ( | ||
<SessionBadge className="border border-warning bg-warning-subtle"> | ||
<Loader size={16} className="text-warning" inline /> | ||
<span className="text-warning ml-2">Starting Session</span> |
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: class ml-2
does not exist. Use ms-1
, me-1
, etc.
client/src/styles/icons/sessions.svg
Outdated
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.
[design] same viewbox comment here
Thanks, @leafty. I've included your code suggestions in the latest commit. |
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 be good after this.
@@ -80,7 +86,7 @@ function ButtonWithMenu(props: ButtonWithMenuProps) { | |||
<> | |||
<DropdownToggle | |||
data-cy="more-menu" | |||
className={`${props.className} ${classes}`} | |||
className={cx(props.className || "", classes, "rounded-end-pill")} |
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.
why have || ""
?
className={cx(props.className || "", classes, "rounded-end-pill")} | |
className={cx(props.className, classes, "rounded-end-pill")} |
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 was a misunderstanding; the cx function can handle undefined values.
<> | ||
<span ref={ref} className={buttonStyles.LinkUnderline}> | ||
<Link className="text-decoration-none" to={to}> | ||
{text} <ArrowRight /> |
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 probably looks better:
{text} <ArrowRight /> | |
{text} | |
<ArrowRight className={cx("bi", "ms-1")} /> |
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.
LGTM 🚀 👏
Tearing down the temporary RenkuLab deplyoment for this PR. |
This PR adds a new project page for Renku v2. It includes:
fix #3098
/deploy renku=release-0.52.x