feat: Add sorting functionality to Sales Log table#36
feat: Add sorting functionality to Sales Log table#36Tiwari-Harshvardhan wants to merge 7 commits into
Conversation
- Make all column headers clickable for sorting - Support ascending/descending toggle - Add visual sort indicators (↕ ↑ ↓) - Improve accessibility with ARIA attributes - Add keyboard navigation support - Optimize performance with useMemo - Handle empty state gracefully Closes #ISSUE_NUMBER
|
@Tiwari-Harshvardhan 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 provided pull request diff introduces significant changes to the 📂 Files Changed
🎭 Code PoemCode is clean, and changes are bright, 🚨 Bugs & Warnings* The code does not handle cases where the `sales` array is `null` or `undefined`. It would be better to add a null check to prevent potential errors. * The `sortedSales` array is not memoized correctly when the `sales` array is updated. This could lead to performance issues if the `sales` array is large. * The `handleSort` function does not handle cases where the `sortField` is not a valid field. It would be better to add a check to prevent potential errors. * Looks solid otherwise!💡 Suggestions & Best Practices* Consider adding a loading indicator when the `sales` array is being fetched or updated. * Use a more robust sorting algorithm, such as the `lodash` library, to handle complex sorting scenarios. * Add more test cases to ensure the sorting functionality works correctly in different scenarios. * Consider using a more modern CSS framework, such as `tailwindcss`, to simplify the styling process. * Use a code formatter, such as `prettier`, to ensure consistent code formatting throughout the project. * Consider adding a `README` file to the project to provide documentation and guidance for other developers.🤖 This review was automatically generated by an AI using the Groq Llama 3 API. Please verify suggestions before applying them. |
|
Hey @Tiwari-Harshvardhan , this is a fantastic implementation! The UI, hover states, and especially the ARIA accessibility improvements are top-tier. However, we have a slight 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. If we do, it will only sort the 10 items on the current page instead of the whole database! Requested Changes:
Let me know if you need any help wiring up the Remix server-side sorting! |
📝 SummaryThe provided pull request diff introduces significant changes to the sales log feature, including the addition of sorting and pagination functionality. The changes affect multiple files, including 📂 Files Changed
🎭 Code PoemSales log updates, a wondrous sight 🚨 Bugs & Architectural ViolationsThe provided code appears to be well-structured and follows the specified architectural rules. However, a few potential issues were identified: * In `sales-table.tsx`, the `renderSortableHeader` function uses the `aria-hidden` attribute on the sort indicator span. While this is correct for accessibility, it's essential to ensure that the sort indicator is still visible to users who rely on visual cues. * The `getSortParams` function in `sales-log.tsx` does not handle cases where the `sort` or `dir` query parameters are missing or invalid. It's recommended to add more robust error handling to handle such scenarios. * The `loader` function in `sales-log.tsx` fetches sales data with pagination and sorting. However, it does not handle cases where the `page` or `pageSize` query parameters are invalid or missing. It's recommended to add more robust error handling to handle such scenarios. Looks solid overall, but these minor issues should be addressed to ensure a robust and accessible implementation.💡 Suggestions & Best PracticesTo further improve the code, consider the following suggestions: * In `sales-table.tsx`, the `renderSortableHeader` function can be optimized by using a more efficient data structure, such as a `Map`, to store the sort indicators. * The `getSortParams` function in `sales-log.tsx` can be improved by using a more robust validation mechanism, such as a schema validation library, to ensure that the `sort` and `dir` query parameters conform to the expected format. * The `loader` function in `sales-log.tsx` can be optimized by using a more efficient data fetching mechanism, such as a caching layer, to reduce the number of database queries. * Consider adding more comprehensive testing to ensure that the sorting and pagination functionality works correctly in different scenarios. * To improve performance, consider using a more efficient data structure, such as a `VirtualizedList`, to render the sales data. |
|
Hey @Tiwari-Harshvardhan, thanks for pushing this update! Unfortunately, there are two major architectural bugs in this implementation that break the page's core functionality:
Since Let me know when you've pushed a fix for these two issues! |
- Restore allSalesMetrics query for accurate lifetime summary cards - Remove margin and profit from sortable fields (calculated values) - Remove fake pagination on calculated fields - Add proper summary prop to SalesSummaryCards - Add non-sortable header styles for Margin and Profit columns This ensures: - Summary cards show lifetime totals, not just current page - Sorting works correctly for database columns only - No fake pagination or misleading data
📝 SummaryThe provided pull request diff introduces significant changes to the sales log feature, including improvements to the sales table, sorting, and pagination. The changes aim to enhance the user experience and provide more intuitive navigation and data visualization. 📂 Files Changed
🎭 Code PoemSales log updates, a wondrous sight 🚨 Bugs & Architectural ViolationsThe provided code changes appear to be well-structured and follow the specified architectural rules. However, a few potential issues were identified: * In `app/blocks/sales-log/sales-table.tsx`, the `renderSortableHeader` function uses the `aria-sort` attribute, but it's not clear if this is properly handled for screen readers. It's recommended to test the accessibility of this feature. * In `app/routes/sales-log.tsx`, the `getSortParams` function does not handle cases where the `sort` or `dir` parameters are missing or invalid. It's recommended to add error handling for these scenarios. * The code uses `parseInt` to parse the `page` parameter, which can lead to unexpected behavior if the parameter is not a valid integer. It's recommended to use a more robust parsing method, such as `Number` or a dedicated parsing library. Looks solid, but these minor issues should be addressed.💡 Suggestions & Best PracticesTo further improve the code, consider the following suggestions: * In `app/blocks/sales-log/sales-table.tsx`, the `SalesTable` component has a complex render method. Consider breaking it down into smaller, more manageable functions to improve readability and maintainability. * In `app/routes/sales-log.tsx`, the `loader` function fetches data from the database using the `prisma` client. Consider adding error handling for database errors and implementing a retry mechanism to improve robustness. * The code uses a mix of `const` and `let` declarations. It's recommended to use `const` by default and only use `let` when necessary to improve code readability and avoid unintended side effects. * Consider adding more comprehensive testing for the sales log feature, including unit tests, integration tests, and end-to-end tests to ensure the feature works as expected. |
|
@Tiwari-Harshvardhan Hey there! I just reviewed your latest commit ( Since a few days have passed, we've merged some other PRs into Could you please pull the latest git fetch origin main
git merge origin/mainOnce you've resolved the conflicts in |
- Keep sorting implementation with URL parameters - Integrate platform fees and shipping costs from main - Use raw SQL query for accurate summary calculations - Maintain non-sortable status for Margin and Profit columns - Preserve all accessibility features
📝 SummaryThe provided pull request diff introduces significant changes to the sales log feature, including the implementation of sorting, pagination, and improved accessibility. The changes affect multiple files, including CSS, TypeScript, and React components. 📂 Files Changed
🎭 Code PoemSales log updates, a wondrous sight 🚨 Bugs & Architectural Violations* The `app/blocks/sales-log/sales-table.tsx` file uses the `aria-hidden` attribute correctly, but it's essential to ensure that all interactive elements have a clear and consistent focus state. * The `app/routes/sales-log.tsx` file uses the `useLoaderData` hook to fetch data, but it's crucial to handle potential errors and loading states to prevent the application from crashing. * The `getSortParams` function in `app/routes/sales-log.tsx` does not validate the `searchParams` object, which could lead to errors if the object is null or undefined. * The `loader` function in `app/routes/sales-log.tsx` uses the `prisma` client to fetch data, but it's essential to ensure that the client is properly configured and authenticated to prevent data inconsistencies. * The `action` function in `app/routes/sales-log.tsx` does not validate the `formData` object, which could lead to errors if the object is null or undefined. Looks solid in terms of architectural rules, as it uses Remix, Prisma, and Vanilla CSS Modules, and places UI components in the correct directory.💡 Suggestions & Best Practices* Consider adding a loading state to the SalesTable component to handle cases where the data is still being fetched. * Use a more robust validation library, such as `zod` or `joi`, to validate the `formData` object and prevent potential errors. * Implement a more efficient pagination system, such as using a cursor-based approach, to reduce the number of database queries. * Use a library like `react-query` to handle data fetching and caching, which can improve performance and reduce the number of database queries. * Consider adding a `try-catch` block to the `loader` function to handle potential errors and prevent the application from crashing. * Use a consistent naming convention throughout the codebase, such as using camelCase or PascalCase for variable and function names. * Consider adding a `README` file to the `app/blocks/sales-log` directory to provide a brief overview of the component and its functionality. |
|
@Tiwari-Harshvardhan You're so close! It looks like when you resolved the merge conflicts, a few TypeScript errors slipped in that are failing the Here is exactly what you need to fix:
You can run |
- Remove unused currentPage and onPageChange props from SalesTable - Change totalSales to totalCount in loader return - Add type casting for sortField and sortDirection - Update Pagination component to use only totalPages prop
📝 SummaryThe provided pull request diff introduces several changes to the sales log feature, including improvements to the sales table, sorting functionality, and pagination. The changes aim to enhance the user experience and provide more intuitive navigation and data visualization. 📂 Files Changed
🎭 Code PoemSales log updates, a new delight 🚨 Bugs & Architectural Violations* The `app/blocks/sales-log/sales-table.tsx` file uses the `aria-hidden` attribute on the sort indicator element, but it is not necessary as the element is already hidden from screen readers using `display: none`. * The `app/routes/sales-log.tsx` file uses the `PrismaClient` instance without checking if it is already initialized, which could lead to multiple instances being created. * The `getSortParams` function in `app/routes/sales-log.tsx` does not handle cases where the `sort` or `dir` parameters are missing or invalid. * The `loader` function in `app/routes/sales-log.tsx` does not handle errors properly, which could lead to unexpected behavior or crashes. * The `SalesTable` component in `app/blocks/sales-log/sales-table.tsx` uses the `role` attribute on the `th` element, but it is not necessary as the element is already a table header cell. * The `SalesTable` component in `app/blocks/sales-log/sales-table.tsx` uses the `tabIndex` attribute on the `th` element, but it is not necessary as the element is not focusable. * The `app/blocks/sales-log/sales-table.module.css` file uses the `@media` queries to apply different styles based on screen size, but it does not consider other types of devices or screen readers.💡 Suggestions & Best Practices* Consider using a more robust sorting algorithm, such as the one provided by the `lodash` library, to handle complex sorting scenarios. * Use a more efficient pagination algorithm, such as the one provided by the `react-paginate` library, to reduce the number of database queries. * Improve the accessibility of the sales table by adding more descriptive text for screen readers and providing a clear indication of the sorting direction. * Consider using a more robust error handling mechanism, such as the one provided by the `error-boundary` library, to handle unexpected errors and crashes. * Use a more consistent naming convention throughout the codebase, such as using camelCase or PascalCase for variable and function names. * Consider using a code formatter, such as Prettier, to maintain a consistent code style throughout the codebase. * Use a more robust testing framework, such as Jest or Mocha, to write unit tests and integration tests for the sales log feature. |
|
Great job! You successfully fixed the TypeScript errors inside the The CI is currently failing on one last error: If you look at const { sales, totalPages, summary, sortField, sortDirection, currentPage, pageSize } = useLoaderData<typeof loader>();You forgot to extract Change line 204 to exactly this: const { sales, summary, sortField, sortDirection, totalCount, pageSize } = useLoaderData<typeof loader>();Push that one line change and the build will turn green, and I will merge this PR immediately! |
📝 SummaryThe provided pull request diff introduces several changes to the sales log feature, including improvements to the sales table, sorting functionality, and pagination. The changes aim to enhance the user experience and provide more intuitive navigation and data visualization. 📂 Files Changed
🎭 Code PoemSales log updates, a wondrous sight 🚨 Bugs & Architectural Violations* The `sales-table.module.css` file has been updated to include new CSS classes, but some of the existing classes have been modified to use `transition` and `transform` properties, which may cause performance issues if not used carefully. * The `sales-table.tsx` file has been updated to include sorting and pagination functionality, but the `getSortIndicator` function uses a hardcoded arrow character (`↕`) which may not be suitable for all languages or cultures. * The `sales-log.tsx` file has been updated to handle sorting and pagination parameters, but the `getSortParams` function does not validate the `sort` and `dir` parameters, which may lead to security vulnerabilities if not properly sanitized. * The `loader` function in `sales-log.tsx` uses a `PrismaClient` instance to fetch data from the database, but it does not handle errors properly, which may lead to unexpected behavior or crashes. * The `action` function in `sales-log.tsx` uses a `PrismaClient` instance to create a new sale, but it does not validate the input data properly, which may lead to security vulnerabilities or data corruption. Looks solid in terms of architectural violations, as the code adheres to the specified rules, including the use of Remix, Prisma, and Vanilla CSS Modules.💡 Suggestions & Best Practices* Consider using a more robust and flexible sorting algorithm, such as the one provided by the `lodash` library. * Use a more efficient data structure, such as a `Map` or an `Object`, to store the sorting and pagination parameters, instead of using URL search parameters. * Validate and sanitize all input data, including the `sort` and `dir` parameters, to prevent security vulnerabilities. * Use a more robust error handling mechanism, such as try-catch blocks or error boundaries, to handle unexpected errors or crashes. * Consider using a library like `react-query` to handle data fetching and caching, instead of using a custom implementation. * Use a consistent naming convention throughout the codebase, such as using camelCase or PascalCase for variable and function names. * Consider adding more tests to the codebase, including unit tests and integration tests, to ensure that the code is working as expected. |
2 participants
Closes #32
Description
This PR adds sorting functionality to the Sales Log table, allowing users to sort by any column (Item, Marketplace, Sale Price, Date, Margin, or Profit).
The implementation includes:
aria-sort,aria-label) for screen reader compatibilityuseMemofor sorted dataWhy this is needed: Users currently cannot sort the Sales Log table, making it difficult to find specific sales or analyze data. This feature provides the same sorting capability already available in the Inventory table, creating a consistent user experience across the application.
Related Issues
Fixes #32
Type of Change
Checklist:
Testing Instructions
/app/sales)Additional Notes