Ewc specific responders availability #229
Conversation
| "husky": "^9.1.7", | ||
| "openapi-typescript": "^7.8.0", | ||
| "postcss": "^8", | ||
| "prettier": "^3.6.2", |
KevinWu098
left a comment
KevinWu098
left a comment
There was a problem hiding this comment.
Nice work! I've given it an initial review based on just the code and the video you've attached. Because there are merge conflicts with main, you'll need to resolve those before our staging environment deploys.
Will give it another pass once you've resolved these comments and the merge conflicts.
| setSelectedBlockIndex: (index) => set({ selectedBlockIndex: index }), | ||
| setSelectionIsLocked: (locked) => set({ selectionIsLocked: locked }), | ||
| setHoveredMember: (member) => set({ hoveredMember: member }), | ||
| setSelectedMember: (members) => set({ selectedMember: members }), |
There was a problem hiding this comment.
| setSelectedMember: (members) => set({ selectedMember: members }), | |
| setSelectedMembers: (members) => set({ selectedMember: members }), |
suggestion: shouldn't this method be plural?
| <li className="text-sm italic text-gray-400"> | ||
| N/A | ||
| </li> | ||
| <li className="text-sm italic text-gray-400"></li> |
There was a problem hiding this comment.
question: why are we rendering nothing in the case where we have no members?
suggestion: if it is an improvement to not indicate there are no responders, you should describe why in your PR and show screenshots. Further, you do not need to have an "else" case for this, as list.map(), where list is empty, would simply render nothing anyways
| > | ||
| {member.displayName} | ||
| <div className="flex items-center gap-2"> | ||
| <input |
There was a problem hiding this comment.
suggestion: this probably should be a shadcn checkbox: https://ui.shadcn.com/docs/components/checkbox
| className={cn( | ||
| selectedMember.includes( | ||
| member.memberId | ||
| ) && "font-semibold" |
| availableMembers.map((member) => ( | ||
| {members.length > 0 ? ( | ||
| members.map((member) => ( | ||
| <li |
There was a problem hiding this comment.
issue: why was this changed? does this affect rendering logic? we still render notAvailableMembers, right? we should clarify what the behavior here should be
| <input | ||
| className="cursor-pointer" | ||
| type="checkbox" |
There was a problem hiding this comment.
repeat: this should be a shadcn checkbox
| }; | ||
|
|
||
| const handleCheckboxChange = (memberId: string) => { | ||
| // Toggle the member in the selectedMember array |
There was a problem hiding this comment.
nit: the best comment is no comment.
To elaborate, your code should be written in a way such that it is clear and self-documenting (i.e. does not need a comment to further clarify it). If this holds true, then comments should be reserved for truly complex, difficult to explain code.
| } else if (selectedMember.length > 0) { | ||
| // When members are selected (checkboxes checked), show their combined schedules | ||
| const selectedInBlock = selectedMember.filter((memberId) => | ||
| block.includes(memberId) | ||
| ); | ||
| if (selectedInBlock.length > 0) { | ||
| // Show with opacity based on how many selected members are available | ||
| const opacity = Math.min( | ||
| 0.9, | ||
| 0.5 + selectedInBlock.length * 0.15 | ||
| ); | ||
| blockColor = `rgba(55, 124, 251, ${opacity})`; | ||
| } else { | ||
| blockColor = "transparent"; | ||
| } | ||
| } else if (numMembers > 0) { | ||
| // Normal group view (no hover, no selections) |
There was a problem hiding this comment.
nit: comments may be redundant here
Hovering over a responder will show their specific availability.
Selecting one or more checkboxes will also show specific availability.
Screen.Recording.2025-12-31.at.7.48.17.PM.mov