Page size preference #23
Conversation
There was a problem hiding this comment.
This PR introduces a mechanism to manage the user's preferred page size for better customization of the application experience. Here's a breakdown of the changes:
-
Fetching Initial Page Size:
- Reads the user's preferred page size from local storage.
- If available, attempts to parse the stored value as JSON.
- In case of parsing error, defaults to "default" and throws an error for debugging purposes.
-
useState and useEffect Integration:
- Utilizes the
useStatehook to manage the current page size, initializing it with the parsed or default value. useEffectto synchronize the selected page size with API requests.
- Utilizes the
-
LocalStorage Interaction:
- Implements a
savePageSizePreferencefunction to store the selected page size in local storage.
- Implements a
These changes enhance the user experience by allowing them to customize their preferred page size. The error handling in the parsing process aims to provide better debugging insights if issues arise.
Please review and provide feedback. Thank you!
I have completed it. |
There was a problem hiding this comment.
PR:
❌ Still doesn't link to the issue: #17
❌ Checklist is incomplete/incorrect (e.g. tests are checked, but there aren't any)
I was also confused by the "How to check", as it didn't seem to refer to any actual functionality, but...
Functionality:
❌ Doesn't actually do anything. Yes it updates local storage, and sets the select accordingly across sessions and page re-loads. But what use is that? It doesn't actually change how many items are in the page.
❓ What about other lists in the UI? Admins can see a list of drafts, should that follow this preference too?
❓ How would you approach this if the requirement was to remember a user's preference across browsers/computers?
UI:
❌ Select isn't consistently styled, doesn't match the other one (10, 20, 50) on the same page
❌ Label isn't consistent either ("Title Case" instead of "Sentence case", includes a colon, no whitespace before control)
❓ Is this the right place for it? It's a long way from the other page-related controls, especially if there are any resources in the list.
More detailed comments on the implementation details below. In general:
- It's not well factored, look for opportunities to split out separate parts rather than add eveything into one file; and
- It's completely untested.
Key:
- ❌ Problem, needs fixing before approval
- ❓ Question, needs answering before approval
- 💡 Suggestion, can be taken or left
| .then(setEnvelope) | ||
| .catch((error) => { | ||
| // Handle the error from the API request as needed | ||
| throw new Error("Error fetching published resources:", error); |
There was a problem hiding this comment.
❌ Throwing an error in this way is generally not helpful - if you want to created a new error "caused by" the previous one it should be:
| throw new Error("Error fetching published resources:", error); | |
| throw new Error("Error fetching published resources:", { cause: error }); |
But if you can't really deal with the error or add any useful information, it's best to let it keep going to wherever it ends up.
| }, [resourceService, searchParams]); | ||
| // Using the selected page size in my API request | ||
| resourceService | ||
| .getPublished({ ...searchParams, pageSize }) |
There was a problem hiding this comment.
❌ What does adding pageSize to the arguments here achieve? If you look at the service implementation:
It destructures page and perPage out, but anything else is ignored. You could also see by looking at the Network tab in your developer tools that the request had no additional parameters. Even if that wasn't true, and it passed pageSize along to the request, the GET /resources endpoint doesn't document a pageSize query parameter and would at best ignore it (search/query parameters tend to be optional, although some API implementations will validate and reject 4xx on unexpected parameters/values).
| <option value="default">Default</option> | ||
| <option value="small">Small</option> | ||
| <option value="large">Large</option> |
There was a problem hiding this comment.
❓ What are these values supposed to represent? Assuming they follow what's already on the page, i.e.:
- Default: 20
- Small: 10
- Large: 50
it would be helpful to include those values, and this is an odd order to put them in.
| <ResourceList resources={resources ?? []} /> | ||
| <Pagination lastPage={lastPage ?? 1} /> | ||
| {/* Create the UI for page size selection */} | ||
| <label htmlFor="pageSize">Page Size: </label> |
There was a problem hiding this comment.
💡 It's good to associated labels with controls, it makes the site more accessible, but if you look at the other form controls in the app you can see they generally use nesting to do that:
tech-products-demo/client/src/components/Pagination/index.js
Lines 107 to 109 in 40bf4ae
(This is also accepted by our linting.)
Also note other labels are "Sentence cased" not "Title Cased".
| const storedPageSize = localStorage.getItem("pageSizePreference"); | ||
| let initialPageSize; | ||
|
|
||
| try { | ||
| initialPageSize = storedPageSize ? JSON.parse(storedPageSize) : "default"; | ||
| } catch (error) { | ||
| // Handle the error as needed | ||
| initialPageSize = "default"; | ||
| throw new Error("Error parsing page size preference:", error); | ||
| } | ||
|
|
||
| const [pageSize, setPageSize] = useState(initialPageSize); | ||
|
|
||
| // This function is to save the selected page-size into local storage | ||
| const savePageSizePreference = (pageSize) => { | ||
| localStorage.setItem("pageSizePreference", JSON.stringify(pageSize)); | ||
| }; |
There was a problem hiding this comment.
💡 This is a lot of code in the middle of the home page to handle something relatively minor (and that might also be needed elsewhere). Don't just jam everything into one file, think about how you can separate out unrelated concerns to make your code easier to read (and test). For example, what if you extracted a service to handle local storage, or maybe a hook:
| const storedPageSize = localStorage.getItem("pageSizePreference"); | |
| let initialPageSize; | |
| try { | |
| initialPageSize = storedPageSize ? JSON.parse(storedPageSize) : "default"; | |
| } catch (error) { | |
| // Handle the error as needed | |
| initialPageSize = "default"; | |
| throw new Error("Error parsing page size preference:", error); | |
| } | |
| const [pageSize, setPageSize] = useState(initialPageSize); | |
| // This function is to save the selected page-size into local storage | |
| const savePageSizePreference = (pageSize) => { | |
| localStorage.setItem("pageSizePreference", JSON.stringify(pageSize)); | |
| }; | |
| const [pageSize, setPageSize] = useLocalStorage("pageSizePreference", "default"); |
Also note some of my other suggestions (error handling, comments*) apply here too.
* if you're going to put comments on functions, use JSDoc so they're structured and useful
|
🍞 Closing as stale (more than one month without updates) |
This is a:
Description
Users will have the ability to select their preferred page size using a drop down UI component.
This selected page size preference will be stored in the local storage of the user's browser for future visits to the application.
The application will utilize the browser's local storage to save and retrieve the user's chosen page size preference.
This enables persistent storage of the selected preference, maintaining it even if the user refreshes the page or returns to the application later.
Overall, merging this new code will enhance the user experience by providing an interactive feature that allows users to customize their browsing experience by selecting their preferred page size for resource display.
Use browser developer tools to inspect the local storage for the application.
Look for the presence of stored data related to the page size preference and confirm that it persists across sessions or page reloads.
Links
Author checklist
mainfrom a branch named<category>/<name>, e.g.feature/edit-spaceshipsorbugfix/restore-oxygen