-
Notifications
You must be signed in to change notification settings - Fork 6
feat: support permissions from the API #3351
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-3351.dev.renku.ch |
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.
Nice work!
Just a generic comment: do we want to show a disabled button for users without permissions, or should we hide the button completely instead? The second option seems the most popular on other similar platforms.
If we keep the current approach, I'm wondering if we should at least remove the button for non-logged users.
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 also reviewed, and everything looks good and worked as I expected in the things I tried in the deployment. 🎉
I will let Lorenzo approve the PR, since he has some additional comments.
This PR does not change the UI, any such consideration should be for another PR after consultation with the product team. |
I'm ok with keeping the approach on this PR. It changes how it looks for anonymous users (at least for a few buttons, didn't check all of them), but that's not a blocker. @ciyer feel free to approve the PR if you have reviewed it since I didn't check the code. Otherwise, I'll do it when I'm back next Tuesday |
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.
🎉
Tearing down the temporary RenkuLab deplyoment for this PR. |
Fixes #3350.
See also: SwissDataScienceCenter/renku-data-services#454, SwissDataScienceCenter/renku-data-services#453.
/deploy renku-data-services=main renku=release-0.59.0