-
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: get namespaces owned only when fork a project #2187
Conversation
You can access the deployment of this PR at https://renku-ci-ui-2187.dev.renku.ch |
0733fdb
to
5f0c894
Compare
5f0c894
to
3a52f26
Compare
3a52f26
to
db37f7a
Compare
db37f7a
to
ea6ad9a
Compare
ea6ad9a
to
414c581
Compare
414c581
to
b8a9832
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 good!
I suggest using a wrapper instead of creating the old "NewProjectCoordinator" in other contexts for the time being -- I added a description inline but I'm also happy to implement it if the comment is not clear.
I've noticed a detail with visibilities: when creating a new project, while re-computing the available visibilities, it doesn't show the temporary loader. The behavior is correct on the Fork tho.
I added a gif to explain it better (the comparison is PR vs DEV since I didn't notice it was already working well on forks also in the PR)
P.S. the local e2e tests are failing; it's probably necessary to update them.
client/src/App.js
Outdated
coordinator={ | ||
new NewProjectCoordinator( | ||
props.client, props.model.subModel("newProject"), props.model.subModel("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.
I understand the reason for this, but it feels a bit weird importing and creating the NewProjectCoordinator
here in the App (or in the AddDatasetNewProject
section).
What about adding a temporary wrapper?
This would mean creating a class NewProjectWrapper
in the "ProjectNew.coordinator.js" file that takes the props, create the coordinator and passes it as a prop to the functional component. And that can be exported as export { NewProjectWrapper as NewProject}
That way, once we remove the wrapper, we don't need to modify the code wherever we use NewProject
but just clean up the "ProjectNew.coordinator.js" content.
P.S: I realize this is harder to explain than to implement. I'm happy to push a commit with this change.
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.
sure, added the wrapper
warnings["namespace"] = "Fetching namespaces..."; | ||
|
||
if (meta.namespace.fetching) | ||
if (projects && projects.fetching) | ||
warnings["title"] = "Fetching projects to avoid duplicated 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.
warnings["title"] = "Fetching projects to avoid duplicated projects..."; | |
warnings["title"] = "Fetching projects to prevent duplicates..."; |
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! When reviewing the code, I noticed a couple of small things. The first one was there before your code, but since we are making changes, I think it would make sense to improve.
Another detail: could you incorporate the fix to the It's just a tiny change, from this line
to this line
|
b8a9832
to
88c9cfd
Compare
thanks team for your review, I added all your suggestions 💪 |
708f2a4
to
4f3d061
Compare
3bf9751
to
d941deb
Compare
d941deb
to
883a712
Compare
883a712
to
a14ca85
Compare
a14ca85
to
64d948d
Compare
64d948d
to
e9b4ab1
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.
The changes look good! There were some problems with the tests because the test user had thousands of projects and some of the actions were taking longer now, but those problems have now been fixed.
Tearing down the temporary RenkuLab deplyoment for this PR. |
PR to fetch namespaces owned only for fork and create projects.
Pending:
/deploy #persist #cypress renku=renku-ui-2.15.0-tests
closes #2027