-
Notifications
You must be signed in to change notification settings - Fork 317
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
Improve Invitation flow #9928
Improve Invitation flow #9928
Conversation
d67d0af
to
7b83a0b
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.
CR passed (other than minor issues raised in comments).
QA not yet done
app/ide-desktop/lib/dashboard/src/modals/InviteUsersModal/InviteUsersSuccess.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/modals/InviteUsersModal/InviteUsersSuccess.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/modals/InviteUsersModal/InviteUsersSuccess.tsx
Outdated
Show resolved
Hide resolved
app/ide-desktop/lib/dashboard/src/modals/InviteUsersModal/InviteUsersForm.tsx
Outdated
Show resolved
Hide resolved
@@ -254,15 +254,58 @@ export default class RemoteBackend extends Backend { | |||
|
|||
/** Invite a new user to the organization by email. */ | |||
override async inviteUser(body: backend.InviteUserRequestBody): Promise<void> { | |||
const path = remoteBackendPaths.INVITE_USER_PATH |
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.
why remove path
?
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.
AFAIR I inlined the varible
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.
yeah i mean that works, but separate path
just makes it consistent with all the other endpoints in this file
app/ide-desktop/lib/dashboard/src/modals/InviteUsersModal/parseUserEmails.ts
Show resolved
Hide resolved
[locale] | ||
) | ||
|
||
const isUserOnMembersPage = decodeURIComponent(search) === `?${membersSearchParams}` |
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.
is there a reason this isn't passed as a boolean
prop?
uhh |
QA ❌ (" |
also remember to change |
QA ❌
Minor UI nits:
|
b5d518e
to
b8df31f
Compare
Addressing your QA remarks:
Why though?
Currently, this button does nothing, I think we can tune up the behavior in this issue: https://github.com/enso-org/cloud-v2/issues/890
Popovers do not dim background because basically, they are tooltips with rich content.
Not sure. I think generally we should use red color when we want to highlight irreversible actions like "Remove" or cancel. Closing the dialog isn't one of them. |
for consistency - none of our other tables have multiline cells
we have a dedicated button elsewhere for deleting the current user, and it comes with plenty of warning
in that case we might want an actual modal instead - the "invite users" dialog is something that:
oh, i meant the traffic light in the top left, so that it matches the macos traffic lights |
We'll have then an empty cell for invitations, which is strange.
Okay
Good idea 👍 |
We just ignore duplicates, not sure we need to warn users... |
Thanks for noticing, will take a look once when we'll work on this feature. Ideally, I think, we should merge the behavior. |
I think I've addressed most of the issues and PR is ready for re-QA |
aa03b2f
to
c0b9c43
Compare
c0b9c43
to
02c8640
Compare
Screen.Recording.2024-05-22.at.14.11.15.mov |
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.
see comment
50b3e3d
to
e457804
Compare
@@ -237,6 +237,8 @@ | |||
|
|||
--menu-entry-padding: 0.25rem; | |||
|
|||
--delete-color: rgba(243 24 10 / 87%); | |||
--cta-color: rgb(250, 108, 8); |
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 causes issues with overriding opacity
Pull Request Description
Tl;dr
Closes: enso-org/cloud-v2#1186
This PR significantly improves the invitation UX and add ability to view/resend/copy/delete invitations
Demo Presentation
CleanShot.2024-05-14.at.16.28.40.mp4
Context:
This Change:
What the change does in the larger context? Specific details to highlight for review:
<space>
,<semicolon>
,<colon>
or<newline>
members
page in settings.Test Plan:
Go over how you plan to test it. Your test plan should be more thorough the riskier the change is. For major changes, I like to describe how I E2E tested it and will monitor the rollout.
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.