-
Notifications
You must be signed in to change notification settings - Fork 15
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
added a modal for sending invitation #406
added a modal for sending invitation #406
Conversation
WalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User
participant TeamManagement
participant InviteUserModal
User->>TeamManagement: Click "Invite Team Member"
TeamManagement->>InviteUserModal: Open Modal
User->>InviteUserModal: Enter Email
User->>InviteUserModal: Select Role
User->>InviteUserModal: Click "Send Invite"
InviteUserModal->>TeamManagement: Invoke onSendInvite
TeamManagement-->>User: Invite Processed
Possibly related PRs
Suggested reviewers
Poem
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Clients/src/presentation/components/Modals/InviteUser/index.tsx (3)
66-66
: Duplicate spacing definitions.You're defining both
p: theme.spacing(4)
andpadding: theme.spacing(12)
on line 66. Thepadding
property overridesp
. Consider removing one of them to avoid confusion and ensure consistent styling.sx={{ - p: theme.spacing(4), padding: theme.spacing(12), ... }}
81-82
: Potential for role-based instructions.Currently, the text says "they will get access to all monitors." If different roles grant different levels of access, you may want to clarify or vary this instruction based on user role. This helps users understand each role's scope more accurately.
84-90
: Enhance email validation feedback.Consider adding more explicit validation or feedback messages to guide the user if they enter an invalid email. This will improve user experience and reduce form submission errors.
Clients/src/presentation/pages/SettingsPage/Team/index.tsx (1)
363-372
: Inline arrow function usage.Using an inline arrow function for
onSendInvite
is acceptable, but consider defining a named function if the logic becomes more complex (e.g., adding error handling or multiple steps). This can improve readability and reusability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Clients/src/presentation/components/Modals/InviteUser/index.tsx
(1 hunks)Clients/src/presentation/pages/SettingsPage/Team/index.tsx
(5 hunks)
🔇 Additional comments (6)
Clients/src/presentation/components/Modals/InviteUser/index.tsx (2)
95-108
: Revisit the default role selection.The dropdown starts with a disabled empty option. If a user fails to select a role, the "Send Invite" button remains disabled. This is correct behavior, but you might want to surface a more descriptive message or highlight it in the UI, ensuring the user understands why the button is disabled.
137-143
: Disable button until fields are valid.Disabling the "Send Invite" button when email or role is empty helps prevent incomplete form submissions. This is a good practice. Just ensure that if you add more fields later, you maintain a consistent validation pattern.
Clients/src/presentation/pages/SettingsPage/Team/index.tsx (4)
26-26
: Appropriate import.Importing
InviteUserModal
is correct. Ensure you keep imports organized if additional components are added.
81-81
: Confirm usage of dashboardValues.
dashboardValues
is being retrieved from context, but onlyvendors.length
is used for table pagination. If there are other properties ofdashboardValues
that should be leveraged here, make sure they’re not accidentally overlooked or removed.
83-83
: Descriptive naming of inviteUserModalOpen.
inviteUserModalOpen
is a clear and self-explanatory name. Good job on using semantic naming for state.
247-247
: Clear call-to-action label."Invite team member" succinctly communicates the button's purpose. Great for user clarity!
const inviteTeamMember = () => { | ||
console.log("Inviting team member"); | ||
setInviteUserModalOpen(true); | ||
}; |
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.
🛠️ Refactor suggestion
Encapsulate inviteTeamMember logic if it grows.
Right now, logging and setting the modal open is straightforward. If you need more comprehensive logic for error handling or backend calls, consider moving it to a separate function or hooking into a context or service for maintainability.
@gorkemcetin |
@melpsh can you please use our components instead of the MUI ones? The text fields don't look like they are our components. Also can you left align this area? |
What do you mean by left-align? @gorkem-bwl , it is already left aligned. |
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.
Actionable comments posted: 1
🧹 Nitpick comments (4)
Clients/src/presentation/components/Modals/InviteUser/index.tsx (4)
34-38
: Export a type for the interface.Consider exporting
InviteUserModalProps
in case other components need to reference this interface for consistent type usage throughout the app.+export interface InviteUserModalProps { isOpen: boolean; setIsOpen: (isOpen: boolean) => void; onSendInvite: (email: string, role: string) => void; }
49-70
: Ensure responsive sizing with breakpoints.You might want to explore MUI’s responsive breakpoints for the modal width, instead of a fixed
450
width, to ensure the modal scales appropriately on smaller devices.
83-92
: Validate form fields before sending invites.You can add more robust validation (e.g., checking for a properly formatted email, disallowing invalid roles) to enhance user experience and data integrity.
Also applies to: 93-103
131-151
: Enable partial states for invitation.Currently, the “Send Invite” button is disabled if either
role
is empty. Consider adding more progressive validation or inline checks to guide the user (e.g., “Please enter a valid email”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Clients/src/presentation/components/Modals/InviteUser/index.tsx
(1 hunks)
🔇 Additional comments (4)
Clients/src/presentation/components/Modals/InviteUser/index.tsx (4)
28-32
: Props interface is clearly defined. Good job!Defining the props interface with the required fields (
isOpen
,setIsOpen
,onSendInvite
) is concise and enhances type safety. Keep it up!
72-82
: Good job on descriptive text.Your instructions for inviting a new team member are concise. This helps users understand the impact of adding a new member.
111-130
: Proper design choice for the “Cancel” button.Using a text button for a Cancel action is intuitive. The styling approach using
sx
is also consistent with MUI's API.
158-158
: Export is well-defined.Default export of
InviteUserModal
is straightforward, which simplifies imports in other files.
import { | ||
Button, | ||
MenuItem, | ||
Modal, | ||
Stack, | ||
Typography, | ||
useTheme, | ||
} from "@mui/material"; |
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.
🛠️ Refactor suggestion
Consider using your project's custom UI components instead of MUI.
Per the feedback in the PR comments, there's a preference for aligning with the existing design system by using the project's custom components. This would ensure visual consistency across the application.
Ah ok then - I thought it's center aligned. Good to go! |
Added a new modal for sending invitation for the users
added a new modal and revoked it as a pop up
issue #404
Summary by CodeRabbit
New Features
InviteUserModal
component for inviting team members.Improvements