Conversation
🦋 Changeset detectedLatest commit: 1a151e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![jit-ci[bot]](https://avatars.githubusercontent.com/in/142441?s=60&v=4){kind=small}
|
@raptisj Can you please add a screenshot/video to help with the review process? |
|
Is this UI final? |
@nikosdouvlis mind that I broke the message into two lines since on the other screens we had two lines as well. If we want all to be one-liners we will have to change/delete other localization keys as well. |
… an organization fix(clerk-js): Add delete confirmation input when users try to delete an organization
raptisj
force-pushed
the
add_delete_confirmation
branch
from
21607f4 to
6c105bb
Compare
panteliselef
left a comment
panteliselef
left a comment
There was a problem hiding this comment.
Looks good, but let's figure out if hasMoreThanOneAdmin is calculated correctly
| }); | ||
| const { navigate } = useRouter(); | ||
|
|
||
| const hasMoreThanOneAdmin = (membershipList || [])?.filter(m => m.role === 'admin')?.length > 1; |
There was a problem hiding this comment.
🚫 I believe this will not work. membershipList will not contain all members
The following code, will fetch memberships with a default limit and a default offset.
const { organization, membership, membershipList } = useCoreOrganization({
membershipList: {},
}); For BAPI the default values are mentioned here. I couldn't find the default values for FAPI
There was a problem hiding this comment.
@panteliselef you are right, we probably need to have the limit higher like 50 or something. Ideally, we would want to filter by role and get only the admins but I know we don't have that functionality yet. cc @chanioxaris
I think we are ok limiting it to 50 since I have the suspicion we don't have an active organization with more members.
There was a problem hiding this comment.
Yes Pantelis is right. I would suggest to ignore this for now, until we introduce support for filtering by role to make it more future proof. I am gonna create a ticket to tackle this asap
There was a problem hiding this comment.
@panteliselef @chanioxaris reverting this change since it will be handled in another ticket by team organization
raptisj
force-pushed
the
add_delete_confirmation
branch
from
6c105bb to
087fef9
Compare
…nization fix(clerk-js): Add confirmation input when users try to leave an organization fix(clerk-js): Allow admin to leave organization if there are more than one fix(clerk-js): Add placeholders in inputs fix(clerk-js): Add placeholders in inputs fix(clerk-js): Revert organization admins count fix(clerk-js): Revert organization admins count
raptisj
force-pushed
the
add_delete_confirmation
branch
from
087fef9 to
1a151e7
Compare
|
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
6 participants
Type of change
Packages affected
@clerk/clerk-js@clerk/clerk-react@clerk/nextjs@clerk/remix@clerk/types@clerk/themes@clerk/localizations@clerk/clerk-expo@clerk/backend@clerk/clerk-sdk-node@clerk/shared@clerk/fastify@clerk/chrome-extensiongatsby-plugin-clerkbuild/tooling/choreDescription
npm testruns as expected.npm run buildruns as expected.Add delete confirmation input as an additional check when doing destructive actions.
The action added when users try to:
Screen.Recording.2023-07-06.at.12.48.05.PM.mov