-
Notifications
You must be signed in to change notification settings - Fork 6
feat: add Session Improvements #3659
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
Conversation
You can access the deployment of this PR at https://renku-ci-ui-3659.dev.renku.ch |
33ac8a8
to
23b76e2
Compare
1d86cf3
to
ce6f270
Compare
23b76e2
to
863ca6e
Compare
ce6f270
to
91eae40
Compare
275eb89
to
8373c13
Compare
85e0f94
to
aa6284e
Compare
8373c13
to
dd36bf5
Compare
a0e42a8
to
695f2d4
Compare
…d organize auxiliary build components into a separate file
b632052
to
b36744e
Compare
<div className={cx("d-flex", "gap-2")}> | ||
<img | ||
src={sessionStyles.sessionIcon} | ||
alt="Session icon indicator" |
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.
Improvement: make the alt text reflect the status of the session
const { launcherId } = useMemo(() => { | ||
if (session == null) { | ||
return { launcherId: undefined }; | ||
} | ||
return { | ||
launcherId: session.launcher_id, | ||
}; | ||
}, [session]); |
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.
const { launcherId } = useMemo(() => { | |
if (session == null) { | |
return { launcherId: undefined }; | |
} | |
return { | |
launcherId: session.launcher_id, | |
}; | |
}, [session]); | |
const launcherId = session?.launcher_id; |
import useSessionStartLink from "./useSessionStartLink.hook"; | ||
|
||
interface SessionStartLinkModalProps { | ||
isOpen: boolean; | ||
launcher: SessionLauncher; | ||
project: Project; | ||
launcherId: Ulid; |
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 just a string for the client:
launcherId: Ulid; | |
launcherId: string; |
}: { | ||
launcher: SessionLauncher; | ||
project: Project; | ||
launcherId: Ulid; |
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.
launcherId: Ulid; | |
launcherId: string; |
@@ -160,7 +161,7 @@ function CodeRepositorySelect({ | |||
inputId="builder-environment-code-repository-select-input" | |||
name={name} | |||
isClearable={false} | |||
isSearchable | |||
isSearchable={false} |
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? It works and is useful for cases with a lot of repositories. This is not an improvement, this is a downgrade!! 😢
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.
Yes, however, users can easily misunderstand how the search logic works in this context. In one case, a user pasted a repository URL into the search bar without realizing that it hadn't matched an existing repository. The input silently switched to an existing repo, and the user didn’t notice the change. As a result, they launched a session using the wrong repository.
To avoid this kind of confusion, our longer-term goal is to include a clear option to add a new repository explicitly when launching a session. For now, though, this change already helps mitigate a major source of user frustration.
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 very much disagree here, look at this project -> https://dev.renku.ch/v2/projects/flora.thiebaut/flora-test-many-repos. How is it an improvement to remove the search bar here?!? With the search feature, I can easily pick which repository I want to use without having to scroll and find the repository I need.
I don't think that the search is causing a major frustration. Its absence is doing that.
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.
Thanks for the feedback! You’re totally right — the search is super handy when there are lots of repositories, and I get why removing it feels like a step back.
That said, in testing, most projects had just one or two repos, and we saw users getting confused. One person pasted a repo URL, and because it didn’t exactly match anything, the input silently picked a different repo — they didn’t notice, and ended up launching the session with the wrong one. That caused a lot of frustration.
I think both situations are frustrating in their own way, which is why we want to fix them separately. Ideally, we’d add something like a clear “+ Add new repository” button that opens a modal or something more obvious.
<ExternalLink | ||
role="text" | ||
url={Links.RENKU_2_HOW_TO_USE_OWN_DOCKER_IMAGE} | ||
title="documentation" | ||
/>{" "} |
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.
Include the box arrow up right icon in the link here.
<InfoAlert dismissible={false} timeout={0}> | ||
<p className="mb-0"> | ||
Please see the{" "} | ||
<ExternalLink |
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.
Same here, include the external link icon.
import { Loader } from "../../../../components/Loader"; | ||
import { Links } from "../../../../utils/constants/Docs.js"; |
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.
import { Links } from "../../../../utils/constants/Docs.js"; | |
import { Links } from "../../../../utils/constants/Docs"; |
"flex-row", | ||
"gap-3", | ||
"text-center", | ||
"text-md-start" |
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.
- show icon external link for documentation - make icon alt status show the current session status - fix vertical alignment in environment selector - use type string for launcherId instead of Ulid
Thanks @leafty for the review! |
if (!status.message) return null; | ||
|
||
const popover = ( | ||
<UncontrolledPopover target={uid} trigger="legacy" placement="bottom"> | ||
<UncontrolledPopover target={ref} trigger="hover" placement="bottom"> |
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 accessibility:
<UncontrolledPopover target={ref} trigger="hover" placement="bottom"> | |
<UncontrolledPopover target={ref} placement="bottom"> |
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.
Ok! Hover or focus works well, still mobile-friendly that way.
@@ -223,16 +232,18 @@ export function SessionListRowStatusExtraDetailsV2({ | |||
return ( | |||
<> | |||
{" "} | |||
<span id={uid} className={cx("text-muted", "cursor-pointer")}> | |||
(Click here for details.) | |||
<span ref={ref} className={cx("text-muted", "cursor-pointer")}> |
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 accessibility:
<span ref={ref} className={cx("text-muted", "cursor-pointer")}> | |
<span ref={ref} className={cx("text-muted", "cursor-pointer")} tabIndex={0}> |
See: https://getbootstrap.com/docs/5.3/components/tooltips/#disabled-elements
onClick={(event) => { | ||
event.stopPropagation(); | ||
}} |
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 not needed, popovers are created outside of the app root.
if (!status.message) return null; | ||
|
||
const popover = ( | ||
<UncontrolledPopover target={uid} trigger="legacy" placement="bottom"> | ||
<UncontrolledPopover target={ref} trigger="hover focus" placement="bottom"> |
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: "focus" does not allow users to select the text inside the popover.
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.
maybe try "hover click" for the trigger
4cf5794
to
7304bd7
Compare
@leafty I reverted the last change because it was causing the side panel to open when clicking inside the popover body. I verified that it's still possible to copy the text inside the popover. |
Tearing down the temporary RenkuLab deplyoment for this PR. |
This PR includes a set of enhancements and UI refinements to improve the session launcher and session-related views:
✨ Features:
Added a documentation link to assist users in setting up a custom environment and remove toggle of Advance settings.

Implemented new proposal for environment icons in the environment selector.
Reordered session launcher options and updated copy text on action buttons.

Added a share link button on the show session page.

Updated copy text for the shared session link

Modified session tiles to show session details on hover instead of on click.

Repositioned error/success messages closer to the update button.

Disabled search functionality in the code repository selector and show code repository name for better UX

🐛 Fixes:
/deploy renku=andrea/session-improvements extra-values=dataService.imageBuilders.enabled=true,dataService.imageBuilders.pushSecretName=harbor-secret,dataService.imageBuilders.outputImagePrefix=harbor.dev.renku.ch/andrea-dev/