-
Notifications
You must be signed in to change notification settings - Fork 6
feat: improve intergations UX for code repos #3900
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
base: main
Are you sure you want to change the base?
Conversation
|
You can access the deployment of this PR at https://renku-ci-ui-3900.dev.renku.ch |
e46de6b to
b29be88
Compare
3d912ed to
410a2e3
Compare
* Generate repository APIs code automatically * Show better feedback on code repositories' status * Block session start for unavailable or unaccessible repos
c114938 to
18f84b8
Compare
|
Note: labelling as "do-not-merge" until the backend has been reviewed and merged. |
| export function RepositoryPermissionsBadge({ | ||
| hasWriteAccess, | ||
| repositoryUrl, | ||
| }: RepositoryPermissionsProps) { |
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.
Instead of recomputing the conditions twice (badgeColor and badgeText), compute the condition once and based on the result assign the color and the text. The logic in this component is written twice which means it will be very easy to make a mistake when updating the logic.
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.
Good point! I merged the 2 separate loops here acc8fbd
| <h3>Repository</h3> | ||
| <p> | ||
| URL:{" "} | ||
| <a |
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't we use an existing external link component here?
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! 2a27b11
| <a | ||
| target="_blank" | ||
| rel="noreferrer noopener" | ||
| href="mailto:[email protected]" |
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.
The email needs to be taken from helm values. We need to stop hard-coding [email protected] in our UI code.
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 agree. That problem needs to be addressed application-wide.
I swapped the hardcoded [email protected] for the RenkuContactEmail variable here, but I also made an issue for the cooldown to get the contact email from helm values #3925
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.
Also update the generate-api and update-api targets.
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.
Good catch! e6122d4
client/src/features/ProjectPageV2/ProjectPageContent/CodeRepositories/repositories.utils.ts
Outdated
Show resolved
Hide resolved
| url: string; | ||
| }; | ||
|
|
||
| const withResponseRewrite = repositoriesGeneratedApi.injectEndpoints({ |
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.
Bad practice: do not tamper with API responses, only perform simple data manipulations in the API code.
You can use shouldInterrupt() when it is needed and where it is needed after getting the API data.
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.
You are right: embedding the shouldInterrupt function call in the API responses isn't ideal.
I updated the logic here e11d33a
| const interruptAlways = !!( | ||
| !repositoryData?.metadata?.pull_permission && | ||
| !(repositoryData?.connection?.status === "connected") | ||
| ); | ||
| const interruptOwner = !!( | ||
| (!repositoryData?.metadata?.pull_permission && | ||
| !(repositoryData?.connection?.status === "connected")) || | ||
| (repositoryData?.metadata?.pull_permission && | ||
| !repositoryData?.metadata?.push_permission && | ||
| !(repositoryData?.connection?.status === "connected")) | ||
| ); |
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.
You don't need to do this, you can have a hasProjectWritePermission in the arguments and return only the needed data.
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.
👍 e11d33a
leafty
left a comment
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.
Forgot to post this recording: the offcanvas get triggered when clicking the for label or selecting the input:
Screen.Recording.2025-11-26.at.09.32.44.mov
|
I checked what we have on |
Co-authored-by: Flora Thiebaut <[email protected]>
Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v5...v6) --- updated-dependencies: - dependency-name: actions/checkout dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* build(deps): bump body-parser from 2.2.0 to 2.2.1 in /client Bumps [body-parser](https://github.com/expressjs/body-parser) from 2.2.0 to 2.2.1. - [Release notes](https://github.com/expressjs/body-parser/releases) - [Changelog](https://github.com/expressjs/body-parser/blob/master/HISTORY.md) - [Commits](expressjs/body-parser@v2.2.0...v2.2.1) --- updated-dependencies: - dependency-name: body-parser dependency-version: 2.2.1 dependency-type: indirect ... Signed-off-by: dependabot[bot] <[email protected]> * build: update to body-parser v2.2.1 --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Mind that this doesnt fully address the review comment. That will require #3925
Thanks for reporting the issue! That actually happens on dev too, I assume it's unrelated to the PR (I don't see changes around those bits). |
| <Label for={`project-${project.id}-edit-repository-url`}> | ||
| Repository URL | ||
| </Label> | ||
| <Label>Repository URL</Label> |
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 a fix, please undo. The label should be linked to the input with the for= attribute.
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 works as a workaround (for me, a quick workaround that improves UX is more important than keeping formal correctness in the code), but I can take it out for now if you wish 🤷
I'll make a separate issue to address the problem -- EDIT: #3926
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.
P.S: working on this, I should be done soon #3900 (comment)
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.
Fix:
In function EditCodeRepositoryModal({, update with the following:
<Modal
size={"lg"}
isOpen={isOpen}
toggle={toggleModal}
centered
onClickCapture={(e) => {
e.stopPropagation();
}}
>This is not a proper fix though, the issue is that it is forbidden to place a clickable button inside a clickable button which is what is happening here ("Edit" is inside the clickable code repository row). If we change that, then this offcanvas issue would not exist.
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.
Also "for me, a quick workaround that improves UX is more important than keeping formal correctness in the code" is wrong because it breaks a fundamental Web affordance which is that form labels need to be linked to their input. Breaking this means breaking fundamental UI expectation for people with accessibility constraints. You may "fix" the issue for people using a mouse but you break the UI for people who may not have access to that input method.
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 added the onClickCapture to the FormGroup element; attaching it to the Modal element disabled a bunch of other interactive elements like the close modal icon and the button on the footer
See 69cb87b
|
@leafty Thank you for the review. I should have addressed everything |
Implements the solution for this ShapeUp pitch https://www.notion.so/renku/Make-users-aware-of-need-to-connect-git-integrations-for-code-repositories-27e0df2efafc8087844ac62252ee423e
The PR includes the following changes:
There will be a follow-up to let users connect in-place instead of redirecting to the Integrations page.
/deploy renku-data-services=main renku=build/code-integrations extra-values=enableInternalGitlab=false