Requests page #80
Requests page #80
Conversation
Fixed a naming inconsistency between get request and find request from one of my previous tickets (whoops) Created information cards with request information on them Kanban then loads these into 4 categories and uses the intersection observer pattern to see when the veiwport contains the end of the last information card and loads more if the cursor has more rows to load
…t and called it RequestInformationCard
…t and called it RequestInformationCard
Dao-Ho
left a comment
Dao-Ho
left a comment
There was a problem hiding this comment.
Great rip! 🔥 For frontend prs, I would always try to include a visual attachment (video of you playing around with the page) to show what it looks like. Also, for a feature like this, I would personally try to get the logic working in the backend first before doing the FE, especially for pagination logic. It sets up a better base for you to connect immediately without having to leave boilerplate code in prod FE. Also, a big comment here is to avoid using inline CSS styling, we're using Tailwind for our FE stack.
| } | ||
|
|
||
| func (r *RequestsHandler) GetRequest(c *fiber.Ctx) error { | ||
| func (r *RequestsHandler) FindRequest(c *fiber.Ctx) error { |
There was a problem hiding this comment.
This is actually correct format here.
Get prefix in the handler is the convention we want. 'Find' prefix should be used in the repository.
Thanks for being conscious of this though 🙏
| api.Route("/request", func(r fiber.Router) { | ||
| r.Post("/", reqsHandler.CreateRequest) | ||
| r.Post("/:id", reqsHandler.GetRequest) | ||
| r.Post("/:id", reqsHandler.FindRequest) |
There was a problem hiding this comment.
Also, while you're at it, can you change this to be
r.Get("/:id", reqsHandler.GetRequest) ?
Sorry seems like I missed this typo when reviewing
| </div> | ||
| </div> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Please avoid using inline styling here, our default convention here is to use tailwind for styling. (lmk if you need help with this)
| style={{ textAlign: 'center', padding: '10px', color: '#999' }} | ||
| > | ||
| No more requests | ||
| </div> |
There was a problem hiding this comment.
I don't think we need this, let's not show anything when you render the entire list
| {hasMore && ( | ||
| <div | ||
| ref={observerTarget} | ||
| style={{ textAlign: 'center', padding: '10px' }} |
There was a problem hiding this comment.
again, no inline CSS styling here
| ref={observerTarget} | ||
| style={{ textAlign: 'center', padding: '10px' }} | ||
| > | ||
| {isLoading && 'Loading more...'} |
There was a problem hiding this comment.
For visual purposes, it's a pretty jarring experience to keep seeing "loading more..." every time you reach the bottom. You should just propagate more, if there are more left to be shown. If you need to display some feedback here (like a loading state), it indicates that our query for the paginated list is inefficient, and we should be fixing that instead.
Description
This sets up the Requests page which displays all of the requests in a Kanban style showing Pending, Assigned, In Progress, Completed
Type of Change
Related Issue(s)
#72
Closes # 72
Related to # 72
What Changed?
-Fixed naming issue with GET request endpoint (GetRequest func name -> FindRequest)
Testing & Validation
How this was tested
Screenshots/Recordings
Unfinished Work & Known Issues
Notes & Nuances
Pre-Merge Checklist
Code Quality
Testing & CI
Documentation
Reviewer Notes
I need help 😞 importing the requests model from our backend folder into files in our clients/web/
no matter what the import path wouldn't work for me T_T