add functionality to allow for multiple TEC attendance picture submission #576
Conversation
|
[diff-counting] Significant lines: 132. |
There was a problem hiding this comment.
The progress towards enhancing TEC attendance feature by introducing the capability for multiple picture submissions is lookin' good; including an "Add" button for uploading several images is a good approach! Maybe, adding error-handling for image uploads and integrating a preview feature for users to review their submissions before finalizing could be implemented!
alyssayzhang
changed the title
…com:cornell-dti/idol into add-ability-to-submit-multiple-tec-requests
| newTeamEventAttendance, | ||
| images[index] | ||
| ); | ||
|
|
||
| const newTeamEventAttendance: TeamEventAttendance = { |
There was a problem hiding this comment.
Do we need both newTeamEventAttendance and newTeamEventCreditAttendance here?
There was a problem hiding this comment.
@andrew032011 the check that did not pass said cannot redeclare block-scoped variable 'newteameventattendance'.
There was a problem hiding this comment.
Right, but those two variables look almost the exact same, do we need both?
There was a problem hiding this comment.
Ah, I see what you meant! I removed one of them as I was cleaning up code
| hoursAttended: teamEvent.hasHours ? Number(hours) : undefined, | ||
| image: `eventProofs/${getNetIDFromEmail(userInfo.email)}/${new Date().toISOString()}`, | ||
| eventUuid: teamEvent.uuid, | ||
| pending: true, |
There was a problem hiding this comment.
Type checker should catch this: we killed off the pending field already in #575.
There was a problem hiding this comment.
Found a bug, here's how to reproduce:
- Increase the number of images upload slots to 2
- Upload image for first
- Upload a different image for second
- Submit
- observe that both images are the same
It has to be done in that order to reproduce it. If you switch around 1 and 2, you won't be able to reproduce it.
Attached a video too to show you what I mean:
Screen.Recording.2024-02-28.at.11.06.10.PM.mov
| @@ -59,6 +81,11 @@ const TeamEventCreditForm: React.FC = () => { | |||
| headerMsg: 'No Image Uploaded', | |||
| contentMsg: 'Please upload an image!' | |||
| }); | |||
| } else if (imageIndex.length !== expectedNumOfImages) { | |||
| Emitters.generalError.emit({ | |||
| headerMsg: 'Unsucessful Image Upload', | |||
There was a problem hiding this comment.
typo: Unsuccessful, or I might actually make the header message "Missing Images"
| @@ -19,6 +19,8 @@ const TeamEventCreditForm: React.FC = () => { | |||
| const [pendingAttendance, setPendingAttendance] = useState<TeamEventAttendance[]>([]); | |||
| const [rejectedAttendance, setRejectedAttendance] = useState<TeamEventAttendance[]>([]); | |||
| const [isAttendanceLoading, setIsAttendanceLoading] = useState<boolean>(true); | |||
| const [imageIndex, setImageIndex] = useState<number[]>([0]); | |||
There was a problem hiding this comment.
I don't think you need this imageIndex, setImageIndex hook to be an array. How about just a number to say how many image upload slots you have?
| value={image ? undefined : ''} | ||
| onChange={handleNewImage} | ||
| /> | ||
| {imageIndex.map((item, index) => ( |
There was a problem hiding this comment.
See above, I don't think you're using item here either. Actually item and index are equal here. Don't we just want to know how many images there are? So the only state we want to be storing is numImagesSlots (or something like that), just a number.
| @@ -49,6 +69,8 @@ const TeamEventCreditForm: React.FC = () => { | |||
| }; | |||
|
|
|||
| const submitTeamEventCredit = () => { | |||
| const expectedNumOfImages = images.length; | |||
There was a problem hiding this comment.
No need to create a variable IMO for this, we can just inline it, since you're only using it once
andrew032011
force-pushed
the
add-ability-to-submit-multiple-tec-requests
branch
from
5c169a0 to
fdb9236
Compare
| max-height: 60vh !important; | ||
| } | ||
|
|
||
| .margin_before_button { |
There was a problem hiding this comment.
@andrew032011 the rest of the css file is snake case
There was a problem hiding this comment.
oh ok! let's leave it
| onChange={handleNewImage} | ||
| /> | ||
| {images.map((image, i) => ( | ||
| <div className="input_container" style={{ marginBottom: '10px' }} key={i}> |
There was a problem hiding this comment.
nit: inline the styling here. also, what's className "input_container"? I don't see it defined in a CSS file
| max-height: 60vh !important; | ||
| } | ||
|
|
||
| .margin_before_button { |
There was a problem hiding this comment.
oh ok! let's leave it
Summary
Notion/Figma Link
https://www.notion.so/Idol-FA23-4899da48384747ba8ef8366f034e5ebf?p=ec88c8354f054dbb868a3e4f91f37c93&pm=s
Test Plan
I tested a multiple image submission for a TEC event and previewed that they were the correct images in the pending section. Also, I tested that the error message would appear for an unsuccessful image(s) upload.