Solution #1156
Solution #1156
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 The code is well-structured and meets the core functionality. While there are a few minor issues, such as the sorting logic and some error messages, they don't significantly impact the overall functionality. Consider addressing these minor points for future improvements. Keep up the good work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| import { App } from './App'; | ||
| import { PeopleProvider } from './store/PeopleContext'; | ||
| import { HomePage } from './Pages/HomePage'; | ||
| import { PeoplePage } from './components/PeoplePage'; |
There was a problem hiding this comment.
The import path for PeoplePage should be ../components/PeoplePage instead of ./components/PeoplePage to match the file structure.
| switch (sex) { | ||
| case 'Male': | ||
| normalizedSex = 'm'; | ||
| break; | ||
| case 'Female': | ||
| normalizedSex = 'f'; | ||
| } |
There was a problem hiding this comment.
The switch statement does not have a default case. It's a good practice to include a default case to handle unexpected values, even if it's just to return the current state or log an error.
| {!isLoading && !errorMsg && filteredPeople.length === 0 && ( | ||
| <p data-cy="noPeopleMessage">{errorMsg}</p> | ||
| )} |
There was a problem hiding this comment.
The condition for displaying the 'no people' message uses errorMsg, which might not be appropriate if there is no error but simply no people to display. Consider using a different message or condition to handle this case.
| function isPersonFemale(person: Person | undefined): boolean { | ||
| return person?.sex === 'f'; | ||
| } |
There was a problem hiding this comment.
The isPersonFemale function checks if a person is female. It currently handles undefined by returning false, which is fine, but consider adding a comment to clarify this behavior for future maintainability.
| const context = useContext(FilterContext); | ||
|
|
||
| if (!context) { | ||
| throw new Error('usePeopleContext must be used within a FilterProvider'); |
There was a problem hiding this comment.
The error message in the useFilters hook should refer to 'FilterProvider' instead of 'usePeopleContext'. This seems to be a copy-paste error.
| if (valueA === null || valueB === null) { | ||
| return 0; |
There was a problem hiding this comment.
The sorting logic currently returns 0 if either valueA or valueB is null. This might not be the desired behavior if you want to handle null values differently, such as placing them at the end of the list. Consider adjusting this logic if needed.
https://tania-kuzmenko.github.io/react_people-table-advanced/