Senghoung/edit review feature #426
Conversation
SenghoungLim
added
frontend
|
Deployed staging instance to https://staging-426.peterportal.org |
|
Just by first look, this seems pretty cool so far! I'm not assigned to review the PR, but I do have a few comments on design. I know what you have is the default style for the modal, but I think you should make it look more like the design and/or the one I made in #419 (for both aesthetics but more importantly consistency). To be more specific, that is:
Also, the edit button from your GIF appears to not be square, which makes the backdrop an oval instead of a circle. To be honest, I think don't think we should have it as a circle at all. Instead, I think we should keep the UI consistent with other parts of PPC and do the same thing like we do in the roadmap – that is, have a transparent button that is of the variant "light/dark depending on theme". |
There was a problem hiding this comment.
Works well except for 2 things:
- reCAPTCHA has no effect, should implement or just omit it for review edits which I think may be safe to do, given that the resource already exists
- Form validation does not reset. If I edit a review then go to edit it again, green outlines and check marks are still there.
Left additional comments, mainly on small refactors and design.
Edit: also there's some leftover margin to the left of the grade dropdown that looks weird. Should remove it for editable review form.
package.json
Outdated
| @@ -10,6 +10,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "dotenv-flow": "^4.0.1", | |||
| "react-icons": "^5.0.1", | |||
There was a problem hiding this comment.
Would prefer not to add another icon pack/dependency just for one icon. We already have react bootstrap icons, let's use one of their icons.
| <div className="edit-pen-icon" onClick={() => editReview(review, course, professor)}> | ||
| <FaPen /> | ||
| </div> |
There was a problem hiding this comment.
Should make this a button not a div for accessibility purposes and then style it appropriately. I agree with Ethan's comments on the button styling.
| setProfessor(props.review?.professorID); | ||
| setCourse(props.review?.courseID); | ||
| } | ||
| }, [showForm]); |
There was a problem hiding this comment.
Add the dependencies to clear the warning. The lint check will allow warnings but our goal should be to have none so let's make sure not to add anymore.
| }, [showForm]); | |
| }, [showForm, cookies.user, props]); |
| }; | ||
| if (content.length > 500) { | ||
| setOverCharLimit(true); | ||
| if (props.editable === false) { |
There was a problem hiding this comment.
Does this need to specifically === false, or can we just:
| if (props.editable === false) { | |
| if (!props.editable) { |
| console.log('Data received from SubReview:', { | ||
| course, | ||
| professor, | ||
| review, | ||
| }); |
There was a problem hiding this comment.
| console.log('Data received from SubReview:', { | |
| course, | |
| professor, | |
| review, | |
| }); |
| }); | ||
| dispatch(setFormStatus(true)); | ||
| document.body.style.overflow = 'hidden'; | ||
| console.log('Edit Review clicked!'); |
There was a problem hiding this comment.
| console.log('Edit Review clicked!'); |
| <Button variant="secondary" onClick={handleClose}> | ||
| Close | ||
| </Button> |
There was a problem hiding this comment.
I would say cancel instead.
| <Button variant="secondary" onClick={handleClose}> | |
| Close | |
| </Button> | |
| <Button variant="secondary" onClick={handleClose}> | |
| Cancel | |
| </Button> |
SenghoungLim
had a problem deploying
to
staging-426
GitHub Actions
Failure
Description
Screenshots
Steps to verify/test this change:
Final Checks:
(optional)
Issues
Closes #273