solution #1175
solution #1175
Conversation
There was a problem hiding this comment.
Great job on implementing the task requirements! 🎉 While there are a few minor issues, such as ensuring non-null values and improving class name readability, these do not significantly impact the functionality. Keep up the good work and consider the feedback for future improvements! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <Route path="people" element={<PeoplePage />}> | ||
| <Route path=":slug?" element={<PeoplePage />} /> |
There was a problem hiding this comment.
The nested route for PeoplePage seems to render PeoplePage twice: once for the people path and again for the optional :slug? parameter. This might lead to rendering issues or unexpected behavior. Consider reviewing the intended behavior and adjusting the route structure if necessary.
| <a className="" href="#/people?sex=m"> | ||
| </SearchLink> | ||
| <SearchLink | ||
| className={classNames({ 'is-active ': sex === Sex.Male })} |
There was a problem hiding this comment.
There is an extra space in the class name 'is-active ' which might cause styling issues. Consider removing the trailing space to ensure consistent styling.
| person.name.toLocaleLowerCase().includes(query.toLowerCase()) || | ||
| person.fatherName?.toLocaleLowerCase().includes(query.toLowerCase()) || | ||
| person.motherName?.toLowerCase().includes(query.toLowerCase()), |
There was a problem hiding this comment.
Consider using toLowerCase() consistently for string comparison to avoid potential issues with locale-specific behavior. Replace toLocaleLowerCase() with toLowerCase() for uniformity.
| return true; | ||
| } | ||
|
|
||
| return centuries.includes(Math.ceil(person.born / 100).toString()); |
There was a problem hiding this comment.
Ensure that the Math.ceil(person.born / 100).toString() calculation correctly represents the century. If there are any edge cases (e.g., year 2000), verify that the logic handles them as expected.
| <NavLink | ||
| to={`${person.slug}`} | ||
| className={classNames({ | ||
| 'has-text-danger': person.sex === 'f', |
There was a problem hiding this comment.
Consider using a more descriptive class name for gender-based styling, such as 'female-text' instead of 'has-text-danger', to improve code readability and maintainability.
| <td> | ||
| {person.mother ? ( | ||
| <NavLink | ||
| to={`${person.mother?.slug}`} |
There was a problem hiding this comment.
Ensure that person.mother?.slug is not null or undefined before using it in the to prop of NavLink. This will prevent potential runtime errors if slug is not available.
| </td> | ||
| <td> | ||
| {person.father ? ( | ||
| <NavLink to={`${person.father?.slug}`}> |
There was a problem hiding this comment.
Ensure that person.father?.slug is not null or undefined before using it in the to prop of NavLink. This will prevent potential runtime errors if slug is not available.
[DEMO LINK] --- https://k0tlik02.github.io/react_people-table-advanced/