-
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: allow user to request membership #1424
Conversation
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.
Looks good to me ! I have added a few comments to improve some parts. Feel free to put them as issues if they are too large to handle in this PR.
Great work on the refactoring of the code ! 💪
src/components/item/sharing/csvImport/ImportUsersWithCSVButton.tsx
Outdated
Show resolved
Hide resolved
src/components/pages/item/accessWrapper/RequestAccessContent.tsx
Outdated
Show resolved
Hide resolved
src/components/pages/item/accessWrapper/RequestAccessContent.tsx
Outdated
Show resolved
Hide resolved
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 this PR, it's a great refactor 🙌 I agree with what Basile said and I also let a few comments. Except for that, it looks good to me 🤠
cypress/e2e/item/authorization/itemLogin/itemLoginSetting.cy.ts
Outdated
Show resolved
Hide resolved
cypress/e2e/item/authorization/itemLogin/itemLoginSetting.cy.ts
Outdated
Show resolved
Hide resolved
src/components/pages/item/accessWrapper/RequestAccessContent.tsx
Outdated
Show resolved
Hide resolved
src/components/pages/item/accessWrapper/RequestAccessContent.tsx
Outdated
Show resolved
Hide resolved
8898158
to
69d9207
Compare
Quality Gate failedFailed conditions |
In this PR I tried to simplify a bit the code, by removing the use of the
ItemLoginAuthorization
black box.It also has:
TODO
close #1423