solution#1839
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a couple of core required behaviors are still missing and they block the task from being considered complete. The NameFilter input is currently uncontrolled and does not read or update the query search param; as a result, people are never filtered by name, motherName, or fatherName based on query, and query is never removed when the input becomes empty, which is explicitly required by the specification. In src/App.tsx and src/components/PeoplePage.tsx/PeopleTable.tsx, the NameFilter should use useSearchParams to set query on change and clear it (pass null or delete the param) when the input becomes empty, with value={searchParams.get('query') ?? ''} to keep the UI in sync with the URL. In src/components/PeopleTable.tsx, the filtering chain currently uses only sex and centuries; you need to read query from searchParams and add a .filter before sorting that does a case‑insensitive match against person.name, person.motherName, and person.fatherName (skipping null values).
Additionally, navigation within the People page does not preserve search params, which violates the requirement to keep filters/sorting when selecting a person or clicking the People link. In src/components/PeopleTable.tsx, both the row name link and PersonLink use Link with to={/people/${person.slug}}, and in src/App.tsx the main People nav link uses to="/people"; all of these should be changed to either SearchLink or a Link with to={{ pathname: '/people/...', search: searchParams.toString() }} so the current search params are kept. Once you implement the query handling in the NameFilter, add the corresponding name/mother/father filtering in PeopleTable, and update person/People links to preserve search params, the rest of your implementation (sex/century filters, 3‑state sorting, conditional sidebar rendering) looks solid and aligned with the requirements. Overall, you’ve done good work on the filtering and sorting foundations; fixing these missing behaviors will bring the solution fully in line with the task spec.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| </Link> | ||
| </div> | ||
| </div> | ||
| </nav> | ||
|
|
||
| <div className="section"> | ||
| <main className="section"> |
There was a problem hiding this comment.
NameFilter is required to update the query search param and remove it when the input is empty, but this input currently has no value or onChange handler, so no URL search param is ever set or cleared. Consider using useSearchParams here to set query on change and pass null when the input becomes empty.
| </div> | ||
| </div> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
Currently the NameFilter input is uncontrolled and does not update the query search param, which is required. You need to capture changes (e.g., with onChange) and update search params so that non‑empty text sets query and empty text removes query.
| <Link | ||
| className={person.sex === 'f' ? 'has-text-danger' : ''} | ||
| to={`/people/${person.slug}`} |
There was a problem hiding this comment.
Navigation to a specific person currently uses a plain Link to /people/${person.slug}, which discards existing search params (filters/sorting). The spec requires keeping search params when selecting a person; consider switching this to use SearchLink or a Link with to={{ pathname: "/people/" + person.slug, search: searchParams.toString() }} so the URL reflects the same filter/sort state.
| <Link | ||
| className={ | ||
| person.sex === 'f' ? 'has-text-danger' : '' | ||
| } | ||
| to={`/people/${person.slug}`} |
There was a problem hiding this comment.
The main table row name link has the same issue as PersonLink: it navigates to /people/${person.slug} without preserving current search params, so filters/sorting are lost. This violates the requirement to keep search params when navigating within the People page.
| {list | ||
| .filter(person => !sex || person.sex === sex) | ||
| .filter(person => { | ||
| const personCentury = Math.ceil(person.born / 100); | ||
|
|
||
| return ( | ||
| centuries.length === 0 || | ||
| centuries.includes(String(personCentury)) | ||
| ); | ||
| }) |
There was a problem hiding this comment.
The filter chain here only uses sex and centuries. There is no use of a query search param and no case‑insensitive filtering by name, motherName, or fatherName, which is required. You should read query via useSearchParams and add another .filter before sorting to implement this behavior.
| return { sort: null, order: null }; | ||
| }; | ||
|
|
||
| <tr data-cy="person"> | ||
| <td> | ||
| <a href="#/people/bernardus-de-causmaecker-1721"> | ||
| Bernardus de Causmaecker | ||
| </a> | ||
| </td> | ||
| <td>m</td> | ||
| <td>1721</td> | ||
| <td>1789</td> | ||
| <td> | ||
| <a | ||
| className="has-text-danger" | ||
| href="#/people/livina-haverbeke-1692" | ||
| > | ||
| Livina Haverbeke | ||
| </a> | ||
| </td> | ||
| <td> | ||
| <a href="#/people/lieven-de-causmaecker-1696"> | ||
| Lieven de Causmaecker | ||
| </a> | ||
| </td> | ||
| </tr> | ||
| const getSortIcon = (param: string) => { | ||
| if (currentSort !== param) { | ||
| return 'fas fa-sort'; |
There was a problem hiding this comment.
The NameFilter input is currently uncontrolled and not connected to search params. Per the description, it must update the query search param with the typed text and remove query when the input is empty. You’ll need to add value={searchParams.get('query') ?? ''} and an onChange that updates query via setSearchParams or SearchLink helpers.
| const getSortParams = (param: string): SearchParams => { | ||
| if (currentSort !== param) { | ||
| return { sort: param, order: null }; | ||
| } | ||
|
|
||
| <tr data-cy="person"> | ||
| <td> | ||
| <a | ||
| className="has-text-danger" | ||
| href="#/people/elisabeth-haverbeke-1711" | ||
| > | ||
| Elisabeth Haverbeke | ||
| </a> | ||
| </td> | ||
| <td>f</td> | ||
| <td>1711</td> | ||
| <td>1754</td> | ||
| <td> | ||
| <a className="has-text-danger" href="#/people/maria-de-rycke-1683"> | ||
| Maria de Rycke | ||
| </a> | ||
| </td> | ||
| <td> | ||
| <a href="#/people/jan-haverbeke-1671">Jan Haverbeke</a> | ||
| </td> | ||
| </tr> | ||
| if (currentOrder === null) { | ||
| return { sort: param, order: 'desc' }; |
There was a problem hiding this comment.
The People nav link goes to /people without preserving search params. The task explicitly requires keeping search params when clicking the People link; consider using SearchLink here or computing a to that includes current searchParams from useLocation/useSearchParams.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a few critical, requirement-breaking issues left.
- Navbar
Peoplelink loses filters/sorting (critical blocker): insrc/App.tsx, thePeoplenavLinkstill points to"/people"without including current search params. This violates the requirement that all search params (filters and sorting) must be preserved when navigating within the People page. To fix this, read the current search (e.g. viauseLocation().searchoruseSearchParams) and change the link to something liketo={{ pathname: '/people', search: location.search }}or use your existingSearchLinkutility. - NameFilter input not controlled by URL (critical blocker): in
src/components/PeopleFilters.tsx, the NameFilter input updates/removesquerybut does not set itsvaluefromsearchParams.get('query') ?? ''. This breaks the requirement that filters be fully shareable and in sync with the URL (e.g. reloading a?query=...URL won’t show the query in the input). Bind the input withvalue={searchParams.get('query') ?? ''}while keeping your currentonChangelogic. - Reset all filters doesn’t clear
query(critical blocker): the “Reset all filters” control inPeopleFiltersclearscenturiesandsexbut leavesqueryintact, so the name filter can remain active after reset. The spec calls for all filters being reset; update theparamspassed toSearchLinkto also setquery: null. - Minor but important robustness fix: in
src/components/PeopleTable.tsx,inputFiltertreats?query=as an empty string, so all rows match because every string includes''. Even though the UI tries to avoid this state, you should defensively handle it by treating an empty string the same as no query (e.g.if (!item) return true;).
Overall, your implementation is very close, and most complex URL/search behaviors are correct. Once you address these points, your solution will align fully with the task requirements; the work so far is solid and just needs these final adjustments.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| <Link | ||
| className={ | ||
| location.pathname.startsWith('/people') | ||
| ? 'navbar-item has-background-grey-lighter' | ||
| : 'navbar-item' | ||
| } | ||
| to={'/people'} |
There was a problem hiding this comment.
The People nav link currently navigates to /people without preserving existing search params, so clicking it from within the People page clears filters/sorting; per the requirements, update this Link to include the current search (e.g. to={{ pathname: '/people', search: location.search }} or use SearchLink) so search params are kept when clicking People.
| params={{ sex: null }} | ||
| > | ||
| All | ||
| </a> | ||
| <a className="" href="#/people?sex=m"> | ||
| </SearchLink> | ||
| <SearchLink | ||
| className={searchParams.get('sex') === 'm' ? 'is-active' : ''} | ||
| params={{ sex: 'm' }} |
There was a problem hiding this comment.
The People nav link always goes to /people and drops any existing search params, but the spec requires that filters/sorting be preserved when clicking People. Consider using useSearchParams or useLocation().search here and pass a to object (or SearchLink) that includes the current search string so the existing params are kept.
| <div className="columns"> | ||
| <div className="column"> | ||
| <PeopleTable list={list} /> | ||
| </div> |
There was a problem hiding this comment.
The People navigation link currently always goes to /people and drops any active search params (filters/sorting). The requirements state that search params must be kept when navigating within the People page, including when clicking this link. Consider building the to using current search params (e.g. via useSearchParams or SearchLink) so filters and sort state are preserved.
| const inputFilter = (person: Person) => { | ||
| const item = searchParams.get('query')?.toLowerCase().trim(); | ||
|
|
||
| if (item === undefined) { | ||
| return true; | ||
| } | ||
|
|
||
| return ( |
There was a problem hiding this comment.
In inputFilter, when query exists but is an empty string (e.g. ?query=), item becomes '' and the subsequent includes checks will all return true (since any string includes an empty substring), so the filter behaves as if everything matches instead of treating query as absent. To align with the requirements and make the logic robust, consider treating empty strings the same as no query (e.g. check if (!item) or specifically handle item === undefined || item === '' before applying the filters).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn.
You implemented the main requirements very well: filters and sorting are driven by URL search params, the People nav link preserves filters, the NameFilter is correctly controlled via searchParams.get('query') ?? '', and the reset button clears all filters including query. One remaining issue is in src/components/PeopleTable.tsx: in your inputFilter, when the URL contains ?query=, item becomes an empty string (''), so name.toLowerCase().includes(item) ends up matching all rows; the requirement says an empty query should act as no filter. To align with that, you can add an early guard like if (!item) return true; right after trimming, so both undefined and '' are treated as “no query.”
Overall, your structure, use of useSearchParams, and handling of sorting and multi-century filters show a solid understanding of URL-driven state. Fixing the small empty-query edge case will make your solution fully robust, but your work so far is strong and moving in the right direction.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| search: searchParams.toString() | ||
| ? `?${searchParams.toString()}` | ||
| : '', | ||
| }} |
There was a problem hiding this comment.
Your use of useLocation and useSearchParams to build the People link to object preserves all current search params when navigating back to /people, satisfying the requirement to keep filters/sorting in the URL when using the People nav link.
| : '', | ||
| }} | ||
| > | ||
| People |
There was a problem hiding this comment.
Binding the input value to searchParams.get('query') ?? '' ensures the NameFilter is controlled by the URL (e.g. persisted on reload), which matches the requirement and fixes the previous review issue.
| > | ||
| 17 | ||
| </a> | ||
| </SearchLink> | ||
|
|
||
| <a | ||
| <SearchLink | ||
| data-cy="century" | ||
| className="button mr-1 is-info" | ||
| href="#/people?centuries=18" | ||
| className={ | ||
| searchParams.getAll('centuries').includes('18') | ||
| ? 'is-info button mr-1' | ||
| : 'button mr-1' | ||
| } | ||
| params={getCenturyParams('18')} | ||
| > |
There was a problem hiding this comment.
In inputFilter, item will be '' (empty string) when the URL has ?query=, and currently you only check for item === undefined. Because every string includes '', this makes all people match, whereas the requirement says empty input / empty query should mean no filter. Consider treating both undefined and '' as no query (e.g., return true early if !item).
| const inputFilter = (person: Person) => { | ||
| const item = searchParams.get('query')?.toLowerCase().trim(); | ||
|
|
||
| if (item === undefined) { |
There was a problem hiding this comment.
item comes from searchParams.get('query')?.toLowerCase().trim(), so if the URL contains ?query= it becomes an empty string '', not undefined. In that case, includes(item) will match all rows. To align with the requirement that empty query means “no filter,” consider treating both undefined and empty string as no query (e.g. early return true if !item).
2 participants
DEMO LINK