Skip to content

react_people-table-advanced#1825

Open
lishvets wants to merge 1 commit into
mate-academy:masterfrom
lishvets:develop
Open

react_people-table-advanced#1825
lishvets wants to merge 1 commit into
mate-academy:masterfrom
lishvets:develop

Conversation

@lishvets

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution because all core functional requirements appear to be implemented correctly: filters and sorting are fully driven by URL search params, the three-step sort cycle works as specified, multi-century selection behaves correctly, and search params are preserved when navigating within the People page via PersonLink and SearchLink. The sidebar visibility logic, name/mother/father case-insensitive filtering, and reset behavior all match the checklist expectations.

The main issue I noticed is in src/components/PeoplePage.tsx, where your sort implementation indexes person1[sort] / person2[sort] with a string and then calls .localeCompare or performs numeric subtraction. This weakens TypeScript’s type safety (it effectively pushes you toward any) and can cause confusion or hidden bugs; it would be better to branch explicitly on each supported sort field (e.g., if (sort === 'name') { ... }) so the compiler can help you. This is important for code quality but does not block the task’s required functionality, so it doesn’t prevent approval. Aside from that type-safety concern and a few minor style points, your implementation is solid and aligned with the task; nice work tying together filters, sorting, and URL state so cleanly.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +49 to +52
switch (sort) {
case 'name':
case 'sex':
return person1[sort].localeCompare(person2[sort]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you index the Person object with an arbitrary sort string and then call .localeCompare or subtract values. This breaks type safety in TypeScript and likely forces the compiler to treat person1 as any. Consider handling each field explicitly (e.g., case 'name': return person1.name.localeCompare(person2.name);, etc.) or otherwise ensuring that sort is a properly typed key of Person so you don’t lose type safety.

Comment on lines +13 to +15
<NavLink
to={{
pathname: `/people/${person.slug}`,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This NavLink explicitly carries over the current search params when navigating to a person’s slug, ensuring filters/sort are preserved within the People page (checklist items #2, #15).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants