-
Notifications
You must be signed in to change notification settings - Fork 9
#3384 part review sidebar thing #3417
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
Conversation
chpy04
left a comment
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.
This looks good so far. Couple things:
- first change the service function to call the google drive API function (and change that function to make sense for files other than images)
- change the endpoint to just take in a file id, that way we can use it generically for submission and review files
| return deletedPopup; | ||
| } | ||
|
|
||
| static async downloadSubmissionFile(submissionId: string): Promise<{ buffer: Buffer; type: string; name: string }> { |
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.
This should be very similar to downloadImage in the onboarding.service file
…er button we had so it just opens a new tab with that logic impl, saving should work deleting a file works (all backend should be hooked up).
chpy04
left a comment
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.
Lit, couple things
| notes, | ||
| completedAt | ||
| completedAt, | ||
| ...(fileIds !== undefined && { fileIds }) |
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.
this might work but in terms of readability can you change it to fileIds ? fileIds : review.fileIds
| @@ -0,0 +1,221 @@ | |||
| import React, { useEffect, useState, useContext, useMemo } from 'react'; | |||
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.
Put this file near our other part review frontend files, either in the folder or in a components folder within the part review folder. src/components is reserved for things that are used across the site
| ); | ||
| }; | ||
|
|
||
| export const useUploadReviewFiles = (reviewId: string) => { |
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.
why is this deleted?
merge with feature
merge with feature
walker-sean
left a comment
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.
| const imagedata = await uploadFile(file); | ||
|
|
||
| if (!imagedata?.id) { | ||
| throw new HttpException(500, 'could not upload image'); |
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.
Nit: not necessarily an image
| static async uploadFile(req: Request, res: Response, next: NextFunction) { | ||
| try { | ||
| if (!req.file) { | ||
| throw new HttpException(400, 'Invalid or undefined image data'); |
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.
Nit: not necessarily an image
| commonMistakes: PartReviewCommonMistake[]; | ||
| onDelete: () => void; | ||
| createPopup: (xCoord: number, yCoord: number, title: string, description?: string) => void; | ||
| custom: boolean; |
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.
maybe make this customComment or something
| }) => { | ||
| const [hovering, setHovering] = useState<boolean>(false); | ||
| const [selelcted, setSelected] = useState<boolean>(true); | ||
| const [textEdittor, setTextEdittor] = useState<boolean>(custom); |
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.
spelling -- also, it would probably be better to call this showCustomTextField or something like that
| custom | ||
| }) => { | ||
| const [hovering, setHovering] = useState<boolean>(false); | ||
| const [selelcted, setSelected] = useState<boolean>(true); |
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.
spelling
| <Box display="flex" alignItems="center"> | ||
| {pdf && <DownloadButton fileId={submission.fileIds[fileIdx]} filename={`${submission.name}_${fileIdx + 1}`} />} | ||
| <Typography variant={'h4'}> | ||
| {submission.name} #{submissionIdx + 1} {review ? ' Review' : ''} |
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.
Maybe do ${submission.name} (submission #${submissionIdx + 1}.
Having a number after the name is a bit confusing
| ); | ||
| }; | ||
|
|
||
| export default PDFViewer; |
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.
Should we support multipage pdfs here? Currently, they can only be uploaded but only one file will be shown. Supporting multipage pdfs also means we need to track on which page popups are made
| newPopup | ||
| }) => { | ||
| const [hovering, setHovering] = useState<boolean>(false); | ||
| const [selelcted, setSelected] = useState<boolean>(false); |
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.
spelling
| }) => { | ||
| const [hovering, setHovering] = useState<boolean>(false); | ||
| const [selelcted, setSelected] = useState<boolean>(false); | ||
| const [editting, setEditting] = useState<boolean>(newPopup); |
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.
spelling
|
Also, we should make sure assignees and reviewers are mutually exclusive |
| }} | ||
| onClick={commentOnClick} | ||
| > | ||
| <AddCommentRoundedIcon sx={{ width: '60%', height: '60%' }} /> |
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.
Could you add tooltips to these icon buttons that just say their action
| <FormLabel>File(s)</FormLabel> | ||
| <Grid container> | ||
| {files.map((file, index) => { | ||
| {fileIds.map((file, index) => { |
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.
Could we have these extend down vertically in a list rather than horizontally?
| <Typography>{displayName(file.name)}</Typography> | ||
| <IconButton onClick={() => removeFile(index)}> | ||
| <Typography>{displayName(files[index].name)}</Typography> | ||
| <IconButton |
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.
Nit: the text and icon button are misaligned
| Upload | ||
| <input | ||
| type="file" | ||
| hidden |
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.
since you can have multiple files, make this a multiple file input
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.
This still needs to be made a multiple input
|
Forgot to add: I think clicking on the part link should bring you to the latest submission/review rather than earliest |
walker-sean
left a comment
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.
2 questions:
- Is there utility in having a status selection when making a review or should it always become in review?
- Will there be an ability to create a review from the part details page?
| Upload | ||
| <input | ||
| type="file" | ||
| hidden |
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.
This still needs to be made a multiple input
| {/* custom comment, common mistake, and file upload buttons for reviews*/} | ||
| {reviewMode && ( | ||
| <Box display={'flex'} flexDirection={'column'}> | ||
| <Box |
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.
Add cursor: pointer to this and the following icon
The ability to start a review will be in the actions button that is being worked on right now in a different ticket. Once that exists we can change the status selection options. Although I am thinking that we will want the options to be review or approve with the idea that if people want to start a review with markups they will go into the part page to do that |


Changes
Notes
In the video I make reviews from the new button on the part page. Typically this will be done from the action button on the review, so reviewers would not have to go and find the review they just made ,but the action button is not done yet
Screenshots
I made a yt video again: https://youtu.be/ljDsP20cHNs
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3384