Cindyrzheng/frontend #25
Conversation
cindyrzheng
requested review from
edwardsabalburo and
1strangequark
and removed request for
edwardsabalburo and
1strangequark
There was a problem hiding this comment.
Hey Cindy, thank you for starting on the frontend. This looks good. I raised a little nitpick about React's TS API in the attached thread that might be worth considering.
Only major comment is that I think the student and professor assignment views aren't doing themselves any favors by being separated in the codebase.
They are so similar that I think we should just refactor them into a single ClassView component that we adjust the appearance of based on application state. For now, we can just make the appearance change based on some props passed to the component, but in the future, we should handle the application state with a context provider or something similar. Maybe cookies. Let me know what you think!
| import { Link } from "react-router-dom"; | ||
|
|
||
| //A card representing a single assignment overview | ||
| const StudentAssignmentCard = (props:{name:string}) => { |
There was a problem hiding this comment.
Not so much a review comment, but what are your thoughts on use of React.FC?
| const StudentAssignmentCard = (props:{name:string}) => { | |
| const StudentAssignmentCard: React.FC<{name: string}> = ({name}) => { |
There was a problem hiding this comment.
never used it before, is it preferrable to use?
There was a problem hiding this comment.
didn't end up using it but i am open to changing it later!
| @@ -0,0 +1,34 @@ | |||
| import React, { useState } from "react"; | |||
There was a problem hiding this comment.
Rather than create a separate assignment card for professors and students, how about we just create a single component that renders based on a property mode = student | faculty?
There was a problem hiding this comment.
Ya I can do that
There was a problem hiding this comment.
Im not super happy with the const stuff I did but I think I'll fix things up once we get something like local storage set up for a logged in user!
frontend/src/App.tsx
Outdated
| <Route path="/professor/class/addassignment"> | ||
| <Route index element={<AddAssignmentView />} /> | ||
| </Route> |
There was a problem hiding this comment.
This is okay for now, but I think in the future we will need to refactor our AddAssignmentView to something akin to a modal to avoid cluttering our app routes.
There was a problem hiding this comment.
ok! lmk when we get to that and then we can work on adding a modal
Adding some views for student and professor that is an overview of a class.