feat: add filter dropdown to inventory (#34)#40
Conversation
|
@1531AdityaRaj 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 filtering system to the inventory management page. It adds two new filters, "Status" and "Condition", which allow users to narrow down the inventory list based on these criteria. The filters are implemented as dropdown menus that appear when the "Filter" button is clicked. 📂 Files Changed
🎭 Code PoemFilters added with care, 🚨 Bugs & Warnings* The `onStatusChange` and `onConditionChange` functions are not debounced, which could lead to performance issues if the user rapidly changes the filter values. * The `filteredItems` function does not handle cases where the `item` object is null or undefined, which could lead to runtime errors. * The `statusFilter` and `conditionFilter` state variables are not validated, which could lead to unexpected behavior if invalid values are set. Looks solid otherwise!💡 Suggestions & Best Practices* Consider adding a debounce to the `onStatusChange` and `onConditionChange` functions to prevent excessive re-renders. * Add null checks to the `filteredItems` function to handle cases where the `item` object is null or undefined. * Validate the `statusFilter` and `conditionFilter` state variables to ensure they only accept valid values. * Consider using a more robust filtering library or framework to handle complex filtering logic. * Add unit tests to cover the new filtering functionality and ensure it works as expected.🤖 This review was automatically generated by an AI using the Groq Llama 3 API. Please verify suggestions before applying them. |
|
Hey @1531AdityaRaj , great work on getting the filter logic hooked up! The React state implementation looks very clean. However, before we can merge this, we need to clean up the styling. In this project, we strictly avoid inline styles to maintain a clean and scalable codebase. Requested Changes: Once you extract those styles into the CSS module, I will get this merged right away! |
|
Hey @1531AdityaRaj , one more quick update! We just merged PR #39 which adds Server-Side Pagination to the tables. Because we are now paginating on the server, we can no longer use client-side filtering via Additional Requested Changes:
Let me know if you need any help with the Remix |
📝 SummaryThe provided pull request diff introduces a filter dropdown to the inventory management page, allowing users to filter items by status and condition. The changes include updates to the 📂 Files Changed
🎭 Code PoemFilters added with care, 🚨 Bugs & Architectural Violations* The code does not include any accessibility attributes (e.g. `aria-hidden`, `role`) on the filter dropdown. * The filter dropdown uses `position: absolute`, which may cause issues with screen readers or other assistive technologies. * The code does not handle cases where the filter dropdown is opened and then the user navigates away from the page. * The `inventory-header.tsx` file uses `useState` to manage the filter state, but it is not clear if this state is being persisted across page reloads or navigation. * The code does not include any loading indicators or error handling for cases where the filter data is being fetched. Looks solid in terms of architectural violations, as the code adheres to the specified rules (Remix, Prisma, Vanilla CSS Modules, etc.).💡 Suggestions & Best Practices* Consider adding accessibility attributes (e.g. `aria-hidden`, `role`) to the filter dropdown to improve accessibility. * Use a more robust method for positioning the filter dropdown, such as using a library like `react-portal` or `react-overlay`. * Implement a way to persist the filter state across page reloads or navigation, such as using `localStorage` or a state management library. * Add loading indicators and error handling for cases where the filter data is being fetched. * Consider using a more functional programming approach, such as using `useMemo` or `useCallback` to memoize the filter functions. * Use a consistent naming convention throughout the codebase. * Consider adding more comments or documentation to explain the purpose and behavior of the code. |
📝 SummaryThe provided pull request diff introduces a new filter dropdown to the inventory management page, allowing users to filter items by status and condition. The filter dropdown is toggled by a button and contains two select menus for status and condition. The selected filters are applied to the inventory table, and the search query is updated accordingly. 📂 Files Changed
🎭 Code PoemFilters added with care, 🚨 Bugs & Architectural Violations* The code does not include any accessibility attributes (e.g., `aria-hidden`, `role`) for the filter dropdown and its components. * The `filterDropdown` class uses `position: absolute`, which may cause issues with the component's positioning and layout. * The `InventoryHeader` component does not handle cases where the `statusFilter` or `conditionFilter` props are not provided. * The `filteredItems` array is not memoized, which may cause unnecessary re-renders of the `InventoryTable` component. * The code does not follow the performance guideline of using `transform` or `opacity` for CSS animations, as the `filterDropdown` class uses `box-shadow` and `border-radius` which may trigger main-thread repaints.💡 Suggestions & Best Practices* Consider adding accessibility attributes to the filter dropdown and its components to improve usability for users with disabilities. * Use a more robust positioning solution for the `filterDropdown` class, such as using a library like `react-portal` or `react-overlay`. * Add default values or error handling for cases where the `statusFilter` or `conditionFilter` props are not provided. * Memoize the `filteredItems` array using `useMemo` to prevent unnecessary re-renders of the `InventoryTable` component. * Consider using `transform` or `opacity` for CSS animations to improve performance and reduce main-thread repaints. * Use a consistent naming convention for variables and functions throughout the codebase. * Consider adding type annotations for function parameters and return types to improve code readability and maintainability. |
|
Hi @rushikesh-bobade, |
📝 SummaryThe provided pull request diff introduces a filter dropdown to the inventory management page, allowing users to filter items by status and condition. The filter dropdown is toggled by a button and contains two select menus for status and condition. The selected filters are applied to the inventory items and the page is updated accordingly. 📂 Files Changed
🎭 Code PoemFilters added with care, 🚨 Bugs & Architectural Violations* The code does not include any accessibility features for the filter dropdown, such as `aria-hidden` or `role` attributes. * The `filterDropdown` class uses `position: absolute`, which may cause issues with the layout if the parent element is not positioned relatively. * The `filterDropdown` class uses `z-index: 100`, which may cause issues with other elements on the page that have a higher `z-index`. * The code does not handle the case where the user selects a filter and then navigates away from the page. The filter will be lost when the user returns to the page. * The code does not include any validation for the filter values. If the user selects an invalid filter value, the page may not update correctly. Looks solid in terms of architectural violations, as it follows the required rules of using Remix, Prisma, and Vanilla CSS Modules, and places UI components in the correct directory.💡 Suggestions & Best Practices* Consider adding accessibility features to the filter dropdown, such as `aria-hidden` and `role` attributes. * Consider using a more robust positioning method for the filter dropdown, such as using a library like `react-portal`. * Consider adding validation for the filter values to ensure that the page updates correctly. * Consider adding a way to persist the filter values when the user navigates away from the page, such as using local storage or a query parameter. * Consider adding a loading indicator while the filter is being applied, to improve the user experience. * Consider using a more efficient data structure for the filter values, such as a `Map` or a `Set`, to improve performance. * Consider adding a way to reset the filters to their default values, to improve the user experience. |
|
Hi @rushikesh-bobade, I've implemented the requested changes and updated the PR. I wanted to test the feature end-to-end, but I don't have access to the project's deployed environment. If possible, could you grant me access or provide a preview/development URL so I can verify the changes before you review them? Thank you! |
|
Hey @1531AdityaRaj ! Fantastic work! The dropdown styles look much better in the CSS module, and your Prisma To test your code, you don't need access to the deployed environment—you can just test it locally! Make sure you have Docker running, then run I just took a look at the code, and there is one tiny thing we need to fix before merging: Requested Change: Once you remove that redundant client-side filter, this is perfect and ready to merge! |
rushikesh-bobade
changed the title
|
Hi @rushikesh-bobade , I've already made the requested changes and pushed the latest commits. The client-side filtering has been removed, and InventoryTable is now receiving the server-filtered items directly. |
|
Hey @1531AdityaRaj! Thanks for the quick update! First, regarding the local setup: I apologize, I misspoke in my last comment! We don't have a
Second, I just took a peek at your latest commit. It looks like the client-side filtering block is actually still in the file! If you look in const filteredItems = items.filter((item) => {
const matchesStatus =
statusFilter === "ALL" || item.status === statusFilter;
const matchesCondition =
conditionFilter === "ALL" || item.condition === conditionFilter;
return matchesStatus && matchesCondition;
});And then in the return statement: <InventoryTable
selected={selected}
onSelectChange={setSelected}
items={filteredItems} {/* <-- This is still using the client-side array! */}
onEdit={setEditingItem}
/>Because your Prisma Give that a quick fix and it'll be good to merge! Let me know if you run into any issues spinning up the local Supabase instance. |
📝 SummaryThe provided pull request diff introduces a filter dropdown feature to the inventory management page. The filter dropdown allows users to filter items by status and condition. The changes include updates to the 📂 Files Changed
🎭 Code PoemFilters added with care, 🚨 Bugs & Architectural ViolationsThe code changes appear to be well-structured and follow the architectural rules. However, there are a few potential issues: * The `filterDropdown` class is not using `aria-hidden` to hide the dropdown from screen readers when it is not visible. This could cause accessibility issues. * The `filterDropdown` class is using `position: absolute` which could cause layout issues if the dropdown is not properly positioned. * The `setStatusFilter` and `setConditionFilter` functions are not checking if the `status` or `condition` parameters are valid before updating the search parameters. This could cause errors if invalid values are passed. * The `loader` function is not handling errors properly. If an error occurs while loading the data, it will not be caught and handled. Looks solid otherwise!💡 Suggestions & Best Practices* Consider adding `aria-hidden` to the `filterDropdown` class to hide it from screen readers when it is not visible. * Consider using a more robust positioning method for the `filterDropdown` class, such as using a library like `react-popper`. * Consider adding validation to the `setStatusFilter` and `setConditionFilter` functions to ensure that only valid values are passed. * Consider adding error handling to the `loader` function to catch and handle any errors that may occur. * Consider using a more efficient method for updating the search parameters, such as using a single `useSearchParams` hook instead of multiple `setSearchParams` calls. * Consider adding a `defer` attribute to the `select` elements to improve performance. |
2 participants
Summary
Implemented the Inventory Filter Dropdown feature.
Changes made
Closes #34
Type of Change
Checklist