Skip to content

Conversation

@PrachodK
Copy link
Contributor

@PrachodK PrachodK commented Jan 6, 2026

Summary

  • Fix attendance status updates using new API endpoint
  • Fix modal layout to wrap months without horizontal scroll
  • Fix present to excused status change
  • Add refresh button to attendance page

… add feature to change from absent to present and vice versa. make dropdown feature for present, absent, excused
@netlify
Copy link

netlify bot commented Jan 6, 2026

Deploy Preview for hackillinois-admin failed.

Name Link
🔨 Latest commit 4435259
🔍 Latest deploy log https://app.netlify.com/projects/hackillinois-admin/deploys/695d80d075adf30008d80c13

@miguelaenlle miguelaenlle self-requested a review January 6, 2026 21:39
Copy link
Contributor

@miguelaenlle miguelaenlle left a 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.

Comment on lines +101 to +106
<Chip
label={team}
icon={<span>💼</span>}
variant="outlined"
sx={{ bgcolor: "white" }}
/>
Copy link
Contributor

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([
Copy link
Contributor

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)
Copy link
Contributor

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)
Copy link
Contributor

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/

Copy link
Contributor

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)}
Copy link
Contributor

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:

  1. Have it be a standalone React component, <StatusSelector /> (or similar), since that's basically what getStatusChip functions as / it's more conventional to just have this be a component.
    • I'd remove this from EventAttendances as well.
  2. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants