-
Notifications
You must be signed in to change notification settings - Fork 1
Fix attendance updates and modal layout #170
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
base: main
Are you sure you want to change the base?
Conversation
… add feature to change from absent to present and vice versa. make dropdown feature for present, absent, excused
❌ Deploy Preview for hackillinois-admin failed.
|
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.
Hey @PrachodK , here are some suggestions I had for the PR.
Before making any changes, I recommend branching onto main, opening a new branch, then manually transferring these changes onto the new branch via git cherry-pick (https://git-scm.com/docs/git-cherry-pick) or just copy/pasting edits.
This is because there are a lot of merge conflicts on this branch since you coded it up a while ago, and I'm concerned about breaking what's on main.
| <Chip | ||
| label={team} | ||
| icon={<span>💼</span>} | ||
| variant="outlined" | ||
| sx={{ bgcolor: "white" }} | ||
| /> |
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 recommend removing this chip for now, since there's no way to modify a user's team yet.
There's a PR that will be merged in to add this functionality, but for now let's hide this since it'll always say "No team"
| const FALL_2025_START = new Date("2025-09-01").getTime() / 1000 | ||
|
|
||
| // Active members list (as of Fall 2025) | ||
| const ACTIVE_MEMBERS_EMAILS = new Set([ |
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.
Let's leave this implementation as-is for now, but isActive will be added to the backend in this PR https://github.com/HackIllinois/adonix/pull/299/changes which will hopefully be merged in soon as priority for the attendance system has increased.
|
|
||
| setAttendances(staffWithStatus) | ||
| } catch (err) { | ||
| console.error(err) |
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.
Display to the user that an error occurred if it did. I recommend using a MUI snackbar: https://mui.com/material-ui/react-snackbar/
|
|
||
| return mandatoryEvents | ||
| } catch (error) { | ||
| console.error("Error fetching events:", error) |
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.
Display to the user if an error occurs. I recommend using a MUI snackbar https://mui.com/material-ui/react-snackbar/
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'd ensure that all the other functions in this file as well will display an error to the user if one occurs.
|
|
||
| return ( | ||
| <Box | ||
| onClick={(e) => handleStatusClick(e, userId, status)} |
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 recommend making these changes:
- Have it be a standalone React component,
<StatusSelector />(or similar), since that's basically whatgetStatusChipfunctions as / it's more conventional to just have this be a component.- I'd remove this from
EventAttendancesas well.
- I'd remove this from
- Make it a dropdown instead of a clickable chip (https://mui.com/material-ui/react-select/), that way it's immediately obvious that it's editable.
Summary