-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/public catalogue UI #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request introduces a public-facing catalogue interface for browsing collection items, though there's a discrepancy with the PR description which mentions volunteer management features that aren't present in the code. The main additions include a complete data fetching layer using React Query, reusable UI components for displaying items, and a search/filter interface for the catalogue.
Key Changes:
- Implemented API client for public items endpoint with filtering capabilities (search, platform, on-floor status, ordering)
- Created reusable catalogue UI components (ItemCard, ItemList, CatalogueSearchBar) using CSS Grid for responsive layout
- Added CataloguePage with search, filter, and real-time data fetching via React Query
- Upgraded Vitest from v2.1.8 to v4.0.16 (major version jump)
Reviewed changes
Copilot reviewed 16 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/vite-env.d.ts |
Defines TypeScript types for Vite environment variables, specifically VITE_API_BASE_URL |
frontend/tsconfig.app.json |
Added explicit reference to vite-env.d.ts for environment variable type safety |
frontend/.env.example |
Example configuration for API base URL environment variable |
frontend/src/lib/types.ts |
Added PublicCollectionItem interface for public API responses |
frontend/src/lib/filters.ts |
Defined ItemFilter interface for query parameters |
frontend/src/lib/constants.ts |
Added ordering options for catalogue sorting |
frontend/src/api/items.api.ts |
Implemented items API client with getAll method and query parameter handling |
frontend/src/api/index.ts |
Exported apiClient and items API for use across the app |
frontend/src/actions/useItems.ts |
Created React Query hook for fetching items with filters |
frontend/src/actions/index.ts |
Exported useItems hook |
frontend/src/components/items/ItemCard.tsx |
Display component for individual collection items with basic styling |
frontend/src/components/items/ItemList.tsx |
Grid-based container component for rendering multiple ItemCards |
frontend/src/components/items/CatalogueSearchBar.tsx |
Search and filter controls for catalogue (search, platform, ordering, on-floor status) |
frontend/src/components/items/index.ts |
Barrel export for all item-related components |
frontend/src/pages/public/CataloguePage.tsx |
Main catalogue page integrating search, filters, and item display with loading/error states |
frontend/src/App.tsx |
Added /catalogue route to application routing |
frontend/package.json |
Upgraded Vitest and related testing packages from v2.1.8 to v4.0.16 |
frontend/package-lock.json |
Updated dependency lockfile reflecting Vitest v4 and transitive dependencies |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import React from 'react'; | ||
| import { useState } from 'react'; | ||
| import { useItems } from '../../actions'; | ||
| import type { ItemFilter } from '../../lib/filters'; | ||
| import { CatalogueSearchBar, ItemList } from '../../components/items' | ||
|
|
||
| const CataloguePage: React.FC = () => { |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The duplicate React import is redundant. Line 1 imports React, and line 2 imports useState from React. Since useState is explicitly imported on line 2, the React namespace import on line 1 is not needed unless it's used elsewhere in the component (which it isn't). Consider removing line 1 to keep imports clean and minimal.
| import React from 'react'; | |
| import { useState } from 'react'; | |
| import { useItems } from '../../actions'; | |
| import type { ItemFilter } from '../../lib/filters'; | |
| import { CatalogueSearchBar, ItemList } from '../../components/items' | |
| const CataloguePage: React.FC = () => { | |
| import { useState, type FC } from 'react'; | |
| import { useItems } from '../../actions'; | |
| import type { ItemFilter } from '../../lib/filters'; | |
| import { CatalogueSearchBar, ItemList } from '../../components/items' | |
| const CataloguePage: FC = () => { |
| <input | ||
| type="text" | ||
| placeholder="Search items..." | ||
| value={filters.search ?? ''} | ||
| onChange={handleSearchChange} | ||
| style={{ | ||
| width: '100%', | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| /> | ||
|
|
||
| <select | ||
| value={filters.platform ?? ''} | ||
| onChange={handlePlatformChange} | ||
| style={{ | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| > | ||
| <option value="">All Platforms</option> | ||
| <option value="PC">PC</option> | ||
| <option value="Switch">Switch</option> | ||
| <option value="Xbox">Xbox</option> | ||
| <option value="PS5">PS5</option> | ||
| </select> | ||
| <select | ||
| value={filters.ordering ?? ''} | ||
| onChange={handleOrderingChange} | ||
| style={{ | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| > | ||
| <option value="">Default Order</option> | ||
| {CATALOGUE_ORDERING_OPTIONS.map((o) => ( | ||
| <option key={o.value} value={o.value}>{o.label}</option> | ||
| ))} | ||
| </select> |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form elements (input and select fields) lack proper accessibility labels. The input field has a placeholder but no associated label element. The select dropdowns and the search input should have proper <label> elements with htmlFor attributes for screen reader users. Consider wrapping each form control with a label or adding separate label elements with proper associations.
| <input | |
| type="text" | |
| placeholder="Search items..." | |
| value={filters.search ?? ''} | |
| onChange={handleSearchChange} | |
| style={{ | |
| width: '100%', | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| /> | |
| <select | |
| value={filters.platform ?? ''} | |
| onChange={handlePlatformChange} | |
| style={{ | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| > | |
| <option value="">All Platforms</option> | |
| <option value="PC">PC</option> | |
| <option value="Switch">Switch</option> | |
| <option value="Xbox">Xbox</option> | |
| <option value="PS5">PS5</option> | |
| </select> | |
| <select | |
| value={filters.ordering ?? ''} | |
| onChange={handleOrderingChange} | |
| style={{ | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| > | |
| <option value="">Default Order</option> | |
| {CATALOGUE_ORDERING_OPTIONS.map((o) => ( | |
| <option key={o.value} value={o.value}>{o.label}</option> | |
| ))} | |
| </select> | |
| <div style={{ display: 'flex', flexDirection: 'column', gap: '0.25rem' }}> | |
| <label htmlFor="catalogue-search-input" style={{ fontSize: '14px' }}> | |
| Search items | |
| </label> | |
| <input | |
| id="catalogue-search-input" | |
| type="text" | |
| placeholder="Search items..." | |
| value={filters.search ?? ''} | |
| onChange={handleSearchChange} | |
| style={{ | |
| width: '100%', | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| /> | |
| </div> | |
| <div style={{ display: 'flex', flexDirection: 'column', gap: '0.25rem' }}> | |
| <label htmlFor="catalogue-platform-select" style={{ fontSize: '14px' }}> | |
| Platform | |
| </label> | |
| <select | |
| id="catalogue-platform-select" | |
| value={filters.platform ?? ''} | |
| onChange={handlePlatformChange} | |
| style={{ | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| > | |
| <option value="">All Platforms</option> | |
| <option value="PC">PC</option> | |
| <option value="Switch">Switch</option> | |
| <option value="Xbox">Xbox</option> | |
| <option value="PS5">PS5</option> | |
| </select> | |
| </div> | |
| <div style={{ display: 'flex', flexDirection: 'column', gap: '0.25rem' }}> | |
| <label htmlFor="catalogue-ordering-select" style={{ fontSize: '14px' }}> | |
| Sort by | |
| </label> | |
| <select | |
| id="catalogue-ordering-select" | |
| value={filters.ordering ?? ''} | |
| onChange={handleOrderingChange} | |
| style={{ | |
| padding: '10px 12px', | |
| fontSize: '16px', | |
| borderRadius: '6px', | |
| border: '1px solid #ccc', | |
| }} | |
| > | |
| <option value="">Default Order</option> | |
| {CATALOGUE_ORDERING_OPTIONS.map((o) => ( | |
| <option key={o.value} value={o.value}>{o.label}</option> | |
| ))} | |
| </select> | |
| </div> |
| {items.map((item) => ( | ||
| <ItemCard key={item.id} item={item} /> | ||
| ))} | ||
| </div> |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ItemList component doesn't implement virtualization for potentially large lists. If the API returns hundreds or thousands of items, rendering all ItemCard components at once could cause performance issues. Consider implementing virtual scrolling using a library like react-window or react-virtualized for better performance with large datasets, or ensure the API implements pagination that the frontend can leverage.
| <div className="catalog-results-section"> | ||
| <h2>Collection Items</h2> | ||
| {isLoading && <p>Loading items...</p>} | ||
| {isError && <p>Failed to load items.</p>} |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error handling is minimal and doesn't provide useful information to users or developers. When isError is true, the component shows "Failed to load items." but doesn't display the actual error message or provide any recovery options. Consider accessing the error from useQuery (e.g., const {data, isLoading, isError, error} = useItems(filters)) and displaying a more helpful error message, possibly with a retry button or troubleshooting steps.
| const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value.trim(); | ||
| setFilters(prev => ({ ...prev, search: value || null })); | ||
| }; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The search input triggers an API call on every keystroke due to the immediate filter update. This could lead to excessive API requests and poor performance. Consider implementing debouncing for the search input to reduce the number of API calls. A typical debounce delay of 300-500ms would improve performance and reduce server load while maintaining good user experience.
| import React from 'react'; | ||
| import { useState } from 'react'; | ||
| import { useItems } from '../../actions'; | ||
| import type { ItemFilter } from '../../lib/filters'; | ||
| import { CatalogueSearchBar, ItemList } from '../../components/items' | ||
|
|
||
| const CataloguePage: React.FC = () => { | ||
|
|
||
| const [filters, setFilters] = useState<ItemFilter>({ | ||
| search: null, | ||
| platform: null, | ||
| is_on_floor: null, | ||
| ordering: null, | ||
| }); | ||
|
|
||
| const {data: items, isLoading, isError} = useItems(filters); | ||
|
|
||
| return ( | ||
| <div style={{ padding: '2rem' }}> | ||
| <h1>Welcome to the Collection Catalog</h1> | ||
| <p>Browse our available items and see what's currently on the floor.</p> | ||
|
|
||
| <div className="search-filter-section" style={{ margin: '2rem 0', width: "auto" }}> | ||
| <h2>Search & Filter</h2> | ||
| <CatalogueSearchBar filters={filters} setFilters={setFilters}/> | ||
| </div> | ||
| <div className="catalog-results-section"> | ||
| <h2>Collection Items</h2> | ||
| {isLoading && <p>Loading items...</p>} | ||
| {isError && <p>Failed to load items.</p>} | ||
| {!isLoading && items?.length === 0 && <p>No items found.</p>} | ||
| {items && items.length > 0 && <ItemList items={items} />} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default CataloguePage; No newline at end of file |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR title and description mention adding "basic component for admin to view volunteers and reject and approve volunteer applications," but no volunteer-related components are present in the actual code changes. The PR only adds catalogue/items functionality (ItemCard, ItemList, CatalogueSearchBar, and CataloguePage). Either the PR description is incorrect, or the volunteer management features are missing from this PR.
| import React from 'react'; | ||
| import { CATALOGUE_ORDERING_OPTIONS } from '../../lib/constants'; | ||
| import type { ItemFilter } from '../../lib/filters'; | ||
|
|
||
| interface SearchBarProps { | ||
| filters: ItemFilter; | ||
| setFilters: React.Dispatch<React.SetStateAction<Partial<ItemFilter>>>; | ||
| } | ||
|
|
||
| const CatalogueSearchBar: React.FC<SearchBarProps> = ({ filters, setFilters }) => { | ||
|
|
||
| const handleSearchChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| const value = e.target.value.trim(); | ||
| setFilters(prev => ({ ...prev, search: value || null })); | ||
| }; | ||
|
|
||
| const handlePlatformChange = (e: React.ChangeEvent<HTMLSelectElement>) => { | ||
| const value = e.target.value.trim(); | ||
| setFilters(prev => ({ ...prev, platform: value || null })); | ||
| }; | ||
|
|
||
| const handleFloorChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| setFilters(prev => ({ | ||
| ...prev, | ||
| is_on_floor: e.target.checked ? true : null, | ||
| })); | ||
| }; | ||
|
|
||
| const handleOrderingChange = (e: React.ChangeEvent<HTMLSelectElement>) => { | ||
| const value = e.target.value.trim(); | ||
| setFilters(prev => ({ ...prev, ordering: value || null })); | ||
| }; | ||
|
|
||
|
|
||
| return ( | ||
| <div | ||
| style={{ | ||
| display: 'flex', | ||
| flexDirection: 'column', | ||
| gap: '1rem', | ||
| maxWidth: '600px', | ||
| width: '100%', | ||
| }} | ||
| > | ||
| <input | ||
| type="text" | ||
| placeholder="Search items..." | ||
| value={filters.search ?? ''} | ||
| onChange={handleSearchChange} | ||
| style={{ | ||
| width: '100%', | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| /> | ||
|
|
||
| <select | ||
| value={filters.platform ?? ''} | ||
| onChange={handlePlatformChange} | ||
| style={{ | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| > | ||
| <option value="">All Platforms</option> | ||
| <option value="PC">PC</option> | ||
| <option value="Switch">Switch</option> | ||
| <option value="Xbox">Xbox</option> | ||
| <option value="PS5">PS5</option> | ||
| </select> | ||
| <select | ||
| value={filters.ordering ?? ''} | ||
| onChange={handleOrderingChange} | ||
| style={{ | ||
| padding: '10px 12px', | ||
| fontSize: '16px', | ||
| borderRadius: '6px', | ||
| border: '1px solid #ccc', | ||
| }} | ||
| > | ||
| <option value="">Default Order</option> | ||
| {CATALOGUE_ORDERING_OPTIONS.map((o) => ( | ||
| <option key={o.value} value={o.value}>{o.label}</option> | ||
| ))} | ||
| </select> | ||
|
|
||
| <label style={{ display: 'flex', alignItems: 'center', gap: '8px', fontSize: '16px' }}> | ||
| <input | ||
| type="checkbox" | ||
| checked={filters.is_on_floor ?? false} | ||
| onChange={handleFloorChange} | ||
| /> | ||
| On Floor | ||
| </label> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| export default CatalogueSearchBar; No newline at end of file |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The component is named "CatalogueSearchBar" using British English spelling "Catalogue", while the API endpoint and other parts of the codebase may use American English "Catalog". For consistency across the codebase, consider standardizing on one spelling convention (typically American English "Catalog" is more common in code). This affects the file name, component name, and any related references.
| } | ||
|
|
||
| const ItemCard: React.FC<ItemCardProps> = ({item}) => { | ||
| console.log(item) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A console.log statement has been left in the production code. This should be removed before merging to production as it can clutter the browser console and may expose internal data structures to end users.
| console.log(item) |
|
|
||
| interface SearchBarProps { | ||
| filters: ItemFilter; | ||
| setFilters: React.Dispatch<React.SetStateAction<Partial<ItemFilter>>>; |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a type mismatch between the SearchBarProps interface and the CataloguePage usage. The SearchBarProps expects setFilters: React.Dispatch<React.SetStateAction<Partial<ItemFilter>>> (with Partial), but CataloguePage passes setFilters from useState<ItemFilter> which has type React.Dispatch<React.SetStateAction<ItemFilter>> (without Partial). This creates a type inconsistency. Either the SearchBarProps should accept ItemFilter instead of Partial<ItemFilter>, or the page should use useState<Partial<ItemFilter>>. Since all filter fields are optional (nullable), using ItemFilter directly would be more appropriate.
| setFilters: React.Dispatch<React.SetStateAction<Partial<ItemFilter>>>; | |
| setFilters: React.Dispatch<React.SetStateAction<ItemFilter>>; |
| getAll: async (params?: ItemFilter): Promise<PublicCollectionItem[]> => { | ||
| const queryParams: Record<string, string> = {}; | ||
| if (params?.platform) queryParams.platform = params.platform; | ||
| if (params?.is_on_floor !== undefined) |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API query parameters handling has a potential bug. When is_on_floor is null, it's correctly not added to the query params. However, when it's false, it would still be excluded (line 9 checks !== undefined, so null would pass but then the condition params.is_on_floor evaluates to false). This means users cannot filter for items that are explicitly NOT on the floor. The condition should be if (params?.is_on_floor !== undefined && params?.is_on_floor !== null) to properly handle all three states: true (on floor), false (not on floor), and null (no filter).
| if (params?.is_on_floor !== undefined) | |
| if (params?.is_on_floor !== undefined && params?.is_on_floor !== null) |
Summary
Adds the basic component for admin to view volunteers and reject and approve volunteer applications.
Sets up basic API interface to access public read endpoints for items, and defines simple starting components for catalogue UI.
Related Issues
Relates to #19 as it pulls data from the defined endpoint.
Closes #9.
Changes
Adds useItems hook and API pathway for /public/items
Added basic component design for ItemCard and ItemList using CSS grid
Introduced shared search bar component with filters and API refreshment upon filter change
Testing
Currently not testable with public items API endpoint as it is still a work in progress at #19.
Screenshots / UI (if applicable)
After: