-
Notifications
You must be signed in to change notification settings - Fork 816
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: Ability to send team invitation in bulk #930
Conversation
@anikdhabal is attempting to deploy a commit to the Documenso Team Team on Vercel. A member of the Team first needs to authorize it. |
Important Auto Review SkippedAuto reviews are limited to the following labels: coderabbit. Please add one of these labels to enable auto reviews. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
Thanks for your review and suggestion. Yeah, the code handles invalid email addresses. It simply shows the error message 'invalid email' in both cases. |
|
||
if (file) { | ||
const reader = new FileReader(); | ||
const emailRegex = /^([A-Z0-9_+-]+\.?)*[A-Z0-9_+-]@([A-Z0-9][A-Z0-9-]*\.)+[A-Z]{2,}$/i; |
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.
Could we not use zod here?
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, we are also using Zod here. We can remove this.
if (file) { | ||
const reader = new FileReader(); | ||
const emailRegex = /^([A-Z0-9_+-]+\.?)*[A-Z0-9_+-]@([A-Z0-9][A-Z0-9-]*\.)+[A-Z]{2,}$/i; | ||
reader.onload = (e) => { |
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 is a bit scary, things like papaparse and such exist for handling the reading of CSV files in a safe manner since the format itself is prone to weirdness.
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 the suggestion. Papaparse would be good for handling csv file.
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.
Consider the bundle size implications as well there may be smaller and just as good options 😄 Papaparse was the most popular last I messed with CSV's but I'm unsure as to what current recommendations are.
https://bundlephobia.com/ is a fantastic website for seeing how big something might be!
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.
Okay😄. Let me check this out.
role: TeamMemberRole.MEMBER, | ||
}, | ||
], | ||
email: '' || [''], |
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.
Seems it would be easier to always make this an array of emails so we don't have to deal with unions. I assume in the UI you would have an option to "Add another member" within the form.
Some thoughts just by looking at the loom (haven't looked at the code)
|
Documenso.and.8.more.pages.-.Personal.-.Microsoft_.Edge.2024-02-16.02-50-11.mp4Thoughts on this? Instead of sending invites from the bulk import section, we use it for importing emails and then redirect to the invite member page, where we can change the selected role before sending the invite. Or, Users can send invitations from both sides, just like the current changes. |
@@ -151,80 +175,123 @@ export const InviteTeamMembersDialog = ({ | |||
An email containing an invitation will be sent to each member. | |||
</DialogDescription> | |||
</DialogHeader> | |||
|
|||
<div className="border-input ring-offset-background flex w-full justify-around rounded-md border"> |
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.
We could have used the Tabs component here. 💭
If we're doing this I'd avoid the text input and just have it only accept CSV where it'll be in We should also supply a template csv which would look like:
Seems crazy but in the past many clients have wanted the template to work off 😄 So to clarify:
|
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, thanks!
fixes #923
Documenso.and.5.more.pages.-.Personal.-.Microsoft_.Edge.2024-02-14.09-49-37.mp4