-
Notifications
You must be signed in to change notification settings - Fork 3
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
Dashboard navbar/frontend #25
Conversation
I have done all the edits and pulled the last changes of the featured branch and everything works correctly.
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.
Nice progress, I've left only smaller comments regarding code readability :)
aria-describedby="modal-modal-description" | ||
> | ||
<Box sx={style} className="modal-container"> | ||
{' '} |
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 can delete this line
@@ -13,28 +15,59 @@ export const AddTeamMemberModal = ({ | |||
if (!showAddModal) { | |||
return null; | |||
} | |||
|
|||
const style = { |
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.
I think this can be moved to the css 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.
You already have the .modal-container
css class
list-style-type: none; /* Remove default list styles */ | ||
padding: 0; /* Remove default padding */ | ||
background: var(--primary-background-color); | ||
padding: 20px 20px 20px 20px; |
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.
There is a short hand for this:
padding: 20px 20px 20px 20px; | |
padding: 20px; |
flex-direction: column; | ||
margin-top: 20px; | ||
background-color: #fff; | ||
padding: 20px 20px; |
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.
padding: 20px 20px; | |
padding: 20px; |
display: flex; | ||
flex-direction: column; | ||
flex: 1 0 0; | ||
margin-left: 30px; |
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.
margin-left: 30px; | |
margin: 0 30px; |
|
||
<div> | ||
<ul> | ||
{teamMembers.map((member) => ( |
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.
Very nice 😌
}) => { | ||
return ( | ||
<li key={member.member_id}> | ||
<div style={{ display: 'flex', alignItems: 'center' }}> |
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.
Please move this to a css class.
None of the companies are using this syntax, since inline css is very difficult to debug & mantain.
…ture-CPH/team26-checkin into frontend-navbar/meraj
Description
I pushed and merged my child's branch to this featured branch (after the edits/comments done). and all works correctly.
Fixes # (issue)
Demo
Checklist