Repository navigation
Conversation
Aokijiop
left a comment
Aokijiop
left a comment
There was a problem hiding this comment.
TL;DL This is pretty solid honestly. Just some nitpicks for styling. I think biggest “issue” is horizontal overflow on smaller screen sizes instead of having the pages be responsive but at this point, I don’t think it’s worth the effort to fix especially since functionality is good. Just keep the bolded stuff on the linked google doc in mind since those were the causes of your styling issues for future projects.
| app.use("/users", usersRouter); | ||
|
|
||
| app.use('/comments', commentsRouter); |
There was a problem hiding this comment.
You guys have a good backend. All data retrieval is done with backend and no additional JS filtering is used which is good practice.
There was a problem hiding this comment.
This comment is more so in general and not really attached to the Navbar in particular but keep in mind that having 100% width + padding is causing overflow in a lot of your guys' pages such as programs and invoices on smaller devices. This problem mostly arises because I noticed almost everywhere, the width is specified with 100% (Spamming this too much in every component such that parent has it → child has it → grandchild has it etc.) can cause overflow and make it really hard to debug the overflow. This also applies to the Navbar for example because it's width is 100% and a lot of the children components on other pages also have width=100% which causes overflow when you add padding.
| .programs-table { | ||
| width: 100%; | ||
| margin: 0px auto 0 auto; | ||
| border: 1px solid var(--Secondary-3, #e2e8f0); | ||
| border-radius: 15px; | ||
| padding: 0px 26px 16px 26px; | ||
| } |
There was a problem hiding this comment.
Use Chakra props for component styling, especially when there’s a built-in component style for it (Ex. Width, height)
Why?
- Compatible with Chakra’s theming so you can take advantage of certain benefits provided by Chakra
- Light/dark mode using the “base” and “_dark” styles
- Responsiveness via built-in breakpoints
| const menuItems = [ | ||
| { | ||
| name: "Programs", | ||
| path: "/programs", | ||
| icon: <NavCalendarIcon />, | ||
| }, | ||
| { | ||
| name: "Invoices", | ||
| path: "/invoices", | ||
| icon: <DocumentIcon />, | ||
| }, | ||
| { | ||
| name: "Notifications", | ||
| path: "/notifications", | ||
| icon: <MdNotifications />, | ||
| count: count, | ||
| fontSize: "xl", | ||
| }, | ||
| { | ||
| name: "Settings", | ||
| path: "/settings", | ||
| icon: <MdSettings />, | ||
| fontSize: "xl", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
You can memoize the rendering of your icons. Check Navbar.jsx and NavBarButton.jsx in the empty branch to see how. This is more so a good standard practice to keep in mind. It'll prob have minimal impact on performance :P
| <Navbar> | ||
| <Box style={{width: "100%", padding: "26px 26px "}}> | ||
| <ProgramSummary | ||
| program={program} | ||
| sessions={sessions} | ||
| bookingInfo={nextBookingInfo} | ||
| isArchived={isArchived} | ||
| setIsArchived={setIsArchived} | ||
| eventId={id} | ||
| /> | ||
| <Sessions | ||
| programName={programName} | ||
| sessions={sessions} | ||
| rooms={roomNames} | ||
| instructors={nextBookingInfo.instructors} | ||
| payees={nextBookingInfo.payees} | ||
| isArchived={isArchived} | ||
| setIsArchived={setIsArchived} | ||
| eventId={id} | ||
| /> | ||
| </Box> | ||
| </Navbar> |
There was a problem hiding this comment.
It's not the best practice to have the Navbar be the parent component for everything as I've noticed in a lot of files. Especially since it's the parent for a lot of other pages, it makes styling harder and bit trickier. Instead, it's recommended instead you have some flex container as the parent so you can style the parent per page by editing this parent and not worry about fixing the formatting in one page but ruining it in another. See Program.jsx in the empty branch for example.
| brand: {}, | ||
| }; | ||
|
|
||
| const theme = extendTheme({ colors }); |
There was a problem hiding this comment.
Also forgot to include in initial review but I noticed you guys were using the default scrollbars. If you want, you can use themes to override the default scrollbar as we've done with past projects. It usually makes it look nicer. I pasted in a template in main.jsx in the empty branch which you guys can use/edit/remove as you wish.
More gcal
184 update gcal
…ding state and toast notifications
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Pool all paid comments for an event, allocate oldest-invoice-first by start_date (then id), and derive payment_status and GET /invoices/paid from allocated amounts so surplus and later-tagged payments cover other periods without duplicating rows. Recalculate all invoices in the event when paid comments change. Shared invoice total helper matches prior /invoices/total logic. Client stops overwriting payment_status via PUT; notifications use paid total field. Tests: node:test for forward rollover, backward credit, and isolated runs. Co-authored-by: Cursor <cursoragent@cursor.com>
InvoiceStats showed getPastDue via amountDue but only getAllDue updated remaining balance after payments; lump sums now refresh both figures. Co-authored-by: Cursor <cursoragent@cursor.com>
Await updateRemainingBalance in handleSaveComment and handleDeleteComment; clear isProcessing in finally so spinner matches paid API round-trip. Co-authored-by: Cursor <cursoragent@cursor.com>
Add GET /invoices/singleInvoiceBalances/:id (pastDue, remainingBalance, paymentStatus) to replace N+1 /total + /paid fan-out from the client. Parallelize invoice total calculation per event; run paid-comments fetch and balance refresh concurrently after save. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Table cells used !important rules in Invoices.css so JSX padding had no effect; bump col 6 padding-right and col 7 padding-left there. Co-authored-by: Cursor <cursoragent@cursor.com>
When allocated > raw and no local paid comment, show em dash plus latest relevant payment date (later billing periods first, else earlier). Reuse event allocation maps on assignments list API. Co-authored-by: Cursor <cursoragent@cursor.com>
API adds coveredByPaymentSource (later|earlier); LAST PAYMENT cell uses two-line copy plus date. Widen column 6 slightly for the new text. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Add total_amount column with sync hooks on bookings, comments, invoices, and room rate changes. loadEventAllocationMaps reads stored totals. Rewrite coverage paid-date query with CTEs. Add backfill script and indexes for invoices/comments. Co-authored-by: Cursor <cursoragent@cursor.com>
…icePayments component
…ments component and enhance notification descriptions
Too lazy to move from Google Doc to here but here is an overview