feat(expenses): add client-side sorting to expenses table#47
feat(expenses): add client-side sorting to expenses table#47BAVYASAKTHIVEL-21 wants to merge 5 commits into
Conversation
|
@BAVYASAKTHIVEL-21 is attempting to deploy a commit to the participationcorner2025-8967's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 SummaryThe pull request introduces a sorting feature to the one-time expenses table. It adds a clickable header with sort indicators and aria-sort attributes for accessibility. The sorting is done using the 📂 Files Changed
🎭 Code PoemSorting headers, a new delight 🚨 Bugs & Architectural Violations* The code uses `var(--color-text-muted)` and other CSS variables, which is acceptable since it's using Vanilla CSS Modules. * The `SortableHeader` component is properly placed inside the `OneTimeExpensesTable` component, and not in a separate file in the `app/components/` directory, which is correct according to the architecture rules. * However, the code does not include any explicit `role` attributes on the `th` elements, which could be added for better accessibility. * The `IconChevronUp`, `IconChevronDown`, and `IconSelector` components are used, but their accessibility is not explicitly handled. It's assumed that these icons are properly implemented with `aria-hidden` attributes. Looks solid, but minor accessibility improvements can be made.💡 Suggestions & Best Practices* Consider adding `role="columnheader"` to the `th` elements for better accessibility. * Add `aria-hidden` attributes to the icon components to ensure they are properly handled by screen readers. * The `useMemo` hook is used correctly to prevent unnecessary re-renders, but it's worth considering using `useCallback` for the `handleSort` function to prevent it from being recreated on every render. * The code uses `var(--color-text-muted)` and other CSS variables, which is acceptable, but it's worth considering using a more explicit naming convention for the CSS variables to improve readability. * The `sortedExpenses` array is created using the `useMemo` hook, but it's worth considering using a more efficient sorting algorithm, such as `lodash.sortBy`, to improve performance. |
|
Hey @BAVYASAKTHIVEL-21 , great work on the table sorting! The UI implementation and accessibility additions look fantastic. However, we have an architectural conflict. We just merged PR #39, which introduced Server-Side Pagination to this exact table. Because the table is now paginated on the server (10 items at a time), we can no longer use client-side sorting with Requested Changes:
Let me know if you need any help wiring up the Remix server-side sorting! |
📝 SummaryThe provided pull request diff introduces sorting functionality to the OneTimeExpensesTable component. It adds a clickable header with sort indicators and aria-sort attributes for accessibility. The sorting is done based on the "date" and "amount" columns, and the direction can be toggled between ascending and descending. The changes also update the loader function to include sorting parameters and the ExpensesTrackerPage component to pass the sorting information to the OneTimeExpensesTable component. 📂 Files Changed
🎭 Code PoemSorting headers, a new delight 🚨 Bugs & Architectural Violations* The `OneTimeExpensesTable` component does not handle the case where the `expenses` prop is `null` or `undefined`. It should add a null check to handle this scenario. * The `SortableHeader` component does not have a `role` attribute, which is required for accessibility. It should add a `role="columnheader"` attribute to the `th` element. * The `IconChevronUp`, `IconChevronDown`, and `IconSelector` components are not wrapped in a `span` element with an `aria-hidden` attribute, which is required for accessibility. It should add a `span` element with `aria-hidden="true"` to wrap these icons. * The `useMemo` hook is used to memoize the sorted expenses, but it does not handle the case where the `expenses` prop changes. It should add a dependency on the `expenses` prop to the `useMemo` hook.💡 Suggestions & Best Practices* The `OneTimeExpensesTable` component can be optimized by using a more efficient sorting algorithm, such as the `stable` sorting algorithm provided by the `Array.prototype.sort()` method. * The `SortableHeader` component can be extracted into a separate component to improve reusability and maintainability. * The `ExpensesTrackerPage` component can be optimized by using a more efficient way to update the sorting information, such as using the `useReducer` hook instead of the `useState` hook. * The code can be improved by adding more comments and documentation to explain the purpose and behavior of each component and function. |
|
Hey @BAVYASAKTHIVEL-21! Awesome job, you nailed the server-side URL sorting and Prisma integration! There’s just a merge conflict right now because another contributor added an "edit/delete" feature to this exact same table while you were working. When you pull Also, great catch by the AI bot—if you could add (P.S. I noticed your commits are showing up as "Unverified". It looks like you're signing them locally but haven't uploaded your GPG public key to your GitHub account settings yet!) |
…add role=columnheader
…add role=columnheader
SummaryThis pull request introduces changes to the FlipTrack application, specifically modifying the Files Changed
ReviewStrengths
Critical Issues - Must Fix Before Merge
Warnings - Should Fix
Suggestions - Nice to Have
Architecture Compliance
VerdictREQUEST CHANGES |
|
This PR has some fantastic updates the implementation in However, there is a critical issue that will break the build. When you pulled Here is how you can resolve this and get the file back to a working state: 1. Fix the Loader Conflict const url = new URL(request.url);
const page = Math.max(1, Number(url.searchParams.get("page")) || 1);
const pageSize = Number(url.searchParams.get("pageSize")) || 10;
const allowedSort = ["date", "amount"] as const;
const sortParam = url.searchParams.get("sort") || "date";
const sort = (allowedSort as readonly string[]).includes(sortParam) ? sortParam : "date";
const dir = url.searchParams.get("dir") === "asc" ? "asc" : "desc";
const countPromise = prisma.expense.count({ where: { userId: user.id } });
const expensesPromise = prisma.expense.findMany({
where: { userId: user.id },
orderBy: { [sort]: dir },
skip: (page - 1) * pageSize,
take: pageSize,
}).then((expenses) =>
expenses.map((e) => ({
...e,
amount: Number(e.amount),
}))
);
// (keep the recurringPromise and aggregatePromise exactly as they are on main)
const deferredData = Promise.all([countPromise, expensesPromise, recurringPromise, aggregatePromise]).then(
([totalExpenses, formattedExpenses, formattedRecurring, sumResult]) => ({
expenses: formattedExpenses,
recurring: formattedRecurring,
totalPages: Math.ceil(totalExpenses / pageSize),
oneTimeTotal: Number(sumResult._sum.amount || 0),
})
);
return { sort, dir, deferredData };2. Fix the Action Conflict if (isRecurring) {
const dayOfMonth = Number(formData.get("dayOfMonth")) || 1;
await prisma.recurringExpense.create({
data: { userId: user.id, type, amount, description, dayOfMonth, isActive: true },
});
} else {
await prisma.expense.create({
data: { userId: user.id, type, amount, description, date },
});
}3. Fix the JSX Conflict <OneTimeExpensesTable
expenses={expenses}
sort={sort as "date" | "amount"}
dir={dir as "asc" | "desc"}
onEdit={(expense) => setEditingExpense(expense)}
/>Once you remove all the |
2 participants
Description
This adds sorting to the one-time expenses table. You can now click on the Date or Amount column header to sort by it, and clicking the same header again flips it between ascending and descending. There's a little arrow next to the header so you can tell which column is sorted and which way.
I set it to sort by Date (newest first) by default, since that's how the table already showed up before, so nothing looks different when the page first loads.
Related Issues
Closes #33
Type of Change
Checklist: