solution #1183

YoNiga7 · 2025-01-05T13:41:05Z

pogorielova

Great work, I like you moved functions outside of components, it looks much cleaner than mine code:)

SemenVodolazskij

good job

mateacademy-ai-mentor

Thank you for your hard work on this project! While your implementation meets many of the task requirements, there are several critical issues that need to be addressed to ensure the application functions correctly and consistently:

Sorting Logic: There are inconsistencies in the sorting logic, such as using the wrong field for sorting in the 'Died' column and handling an empty string as a sort state. These need to be corrected to ensure reliable sorting behavior.
Route Configuration: Some routes, like <Route path=":slug" /> and <Route index />, lack specified elements, which could lead to routes that do nothing when accessed. It's important to provide components or redirects for these routes.
Event Handling: The onClick event handling for SearchLink and SortLink needs to be reviewed to prevent event propagation issues and ensure search parameters are updated correctly.
Parent Lookup: Ensure that the findParent function returns valid objects to avoid runtime errors when accessing properties like .slug.
Conditional Rendering: There are overlapping conditions for rendering messages when no data is available. These should be consolidated to avoid redundancy and ensure clarity.

Please address these issues to improve the functionality and reliability of your application. Keep up the great work, and don't hesitate to reach out if you need further assistance! 😊

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2025-01-06T11:59:04Z

src/Root.tsx

+        <Route index element={<HomePage />} />
+        <Route path="home" element={<Navigate to="/" />} />
+        <Route path="people" element={<PeoplePage />}>
+          <Route index />


The <Route index /> inside the people route does not specify an element. This will result in a route that does nothing when accessed. Consider providing a component or redirect for this index route.

mateacademy-ai-mentor · 2025-01-06T11:59:04Z

src/Root.tsx

+        <Route path="home" element={<Navigate to="/" />} />
+        <Route path="people" element={<PeoplePage />}>
+          <Route index />
+          <Route path=":slug" />


The <Route path=":slug" /> does not specify an element. This will result in a route that does nothing when accessed. Consider providing a component to render when this route is matched.

mateacademy-ai-mentor · 2025-01-06T11:59:04Z

src/components/PeopleFilters.tsx

+    return selectedCenturies.includes(century)
+      ? selectedCenturies.filter(centuryYear => centuryYear !== century)
+      : [...selectedCenturies, century];


The toggleCenturies function is intended to toggle the presence of a century in the selectedCenturies array. However, the function currently returns a new array but does not update the search parameters with this new array. Consider updating the search parameters with the new array returned by toggleCenturies.

mateacademy-ai-mentor · 2025-01-06T11:59:05Z

src/components/PeopleFilters.tsx

+                onClick={() => toggleCenturies(`${century}`)}
+                params={{ centuries: toggleCenturies(`${century}`) }}


The onClick handler for the SearchLink is calling toggleCenturies, but this does not update the search parameters. Ensure that the setSearchParams function is called with the updated centuries list to reflect the changes in the URL.

mateacademy-ai-mentor · 2025-01-06T11:59:05Z

src/components/PeoplePage.tsx

+              {!loading && !error && people.length === 0 && (
+                <p data-cy="noPeopleMessage">
+                  There are no people on the server
+                </p>
+              )}


The condition !loading && !error && people.length === 0 is intended to display a message when there are no people on the server. However, this condition overlaps with the condition on line 62, which also checks for !people.length. Consider combining these conditions to avoid redundancy and ensure the correct message is displayed.

mateacademy-ai-mentor · 2025-01-06T11:59:05Z

src/components/PersonLink.tsx

+        </td>
+        <td>
+          {person.fatherName && names.includes(person.fatherName) ? (
+            <Link to={`/people/${findParent(person.fatherName)?.slug}`}>


Similar to the mother's link, ensure that findParent(person.fatherName) returns a valid Person object to avoid potential null or undefined errors when accessing .slug.

mateacademy-ai-mentor · 2025-01-06T11:59:06Z

src/components/SortLink.tsx

+    sort: field === sortField && isReversed ? null : field,
+    order: field === sortField && !isReversed ? 'desc' : null,


The logic for setting params may lead to unexpected behavior. When field === sortField and isReversed is true, both sort and order are set to null, which might not be the intended behavior. Consider revising the logic to ensure consistent sorting behavior.

mateacademy-ai-mentor · 2025-01-06T11:59:06Z

src/components/SortLink.tsx

+        onClick={() => {
+          onSort();
+        }}


The onClick event is attached to the <span> element, which might cause event propagation issues if the SearchLink component also handles clicks. Consider using event.stopPropagation() if necessary to prevent unintended behavior.

mateacademy-ai-mentor · 2025-01-06T11:59:06Z

src/utils/getFilteredPeople.ts

+    if (name) {
+      const personName = person.name.toLowerCase();
+      const motherName = person.motherName?.toLowerCase();
+      const fatherName = person.fatherName?.toLocaleLowerCase();


There is a typo in toLocaleLowerCase(). It should be toLowerCase() to match the other string methods used for case-insensitive comparison.

mateacademy-ai-mentor · 2025-01-06T11:59:06Z

src/utils/getFilteredPeople.ts

+    }
+
+    if (centuries.length) {
+      const personCentury = Math.ceil(person.born / 100);


Ensure that person.born is always a valid number to avoid potential runtime errors when calculating personCentury. Consider adding validation or default values if necessary.

mateacademy-ai-mentor

Review Summary

Great job on your progress with the application! 🎉 Your implementation shows a solid understanding of React components and routing. However, there are a few areas that need attention to ensure the application functions smoothly:

Sorting Logic: The SortLink component needs a review to ensure the params object correctly updates the sort and order parameters. Make sure the sorting behavior is consistent across all columns, including the 'Died' column.
Route Configuration: The Root.tsx file requires elements for <Route index /> and <Route path=":slug" />. This will ensure proper navigation and functionality when these routes are accessed.
Event Handling: Ensure that event handlers, such as onClick for the 'All' button and onChange for search queries, manage event propagation correctly and reset filters as expected.
Parent Lookup: The findParent function should be robust enough to handle cases where no match is found, preventing runtime errors. Consider adding default values or error handling.
Conditional Rendering: Review the logic for displaying messages when no data is available to avoid overlapping conditions and ensure clarity.
Error Handling: Enhance error handling to provide users with more informative feedback, which will improve the user experience and aid in debugging.

The decision to approve your pull request is to encourage you to continue learning and improving your code. Please address these issues in future iterations to enhance the application's functionality and robustness. Keep up the great work, and use this feedback to refine your skills! 🚀

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

mateacademy-ai-mentor · 2025-01-06T19:53:06Z

src/Root.tsx

+        <Route index element={<HomePage />} />
+        <Route path="home" element={<Navigate to="/" />} />
+        <Route path="people" element={<PeoplePage />}>
+          <Route index />


The <Route index /> is missing an element. It should specify a component or redirect to ensure it performs an action when accessed.

mateacademy-ai-mentor · 2025-01-06T19:53:06Z

src/Root.tsx

+        <Route path="home" element={<Navigate to="/" />} />
+        <Route path="people" element={<PeoplePage />}>
+          <Route index />
+          <Route path=":slug" />


The <Route path=":slug" /> is missing an element. Ensure to provide a component or redirect to handle this route properly.

mateacademy-ai-mentor · 2025-01-06T19:53:07Z

src/components/PeopleFilters.tsx

@@ -22,6 +64,7 @@ export const PeopleFilters = () => {
            type="search"
            className="input"
            placeholder="Search"
+            onChange={handleQueryChange}


The onChange event handler handleQueryChange should prevent default behavior to avoid any unexpected behavior during input changes.

mateacademy-ai-mentor · 2025-01-06T19:53:07Z

src/components/PeopleFilters.tsx

+              className={classNames('button is-success', {
+                'is-outlined': selectedCenturies.length !== 0,
+              })}
+              onClick={() => setSearchParams(new URLSearchParams(searchParams))}


The onClick event handler for the 'All' button should reset the centuries filter. Consider clearing the 'centuries' parameter in the URLSearchParams to ensure all filters are reset.

mateacademy-ai-mentor · 2025-01-06T19:53:07Z

src/components/PeoplePage.tsx

+
+    getPeople()
+      .then(response => setPeople(response))
+      .catch(() => setError(true))


Consider providing more detailed error handling to inform the user about the nature of the error. This can help in debugging and improving user experience.

mateacademy-ai-mentor · 2025-01-06T19:53:07Z

src/components/PersonLink.tsx

+          {person.motherName && names.includes(person.motherName) ? (
+            <Link
+              className="has-text-danger"
+              to={`/people/${findParent(person.motherName)?.slug}`}


Ensure that findParent(person.motherName)?.slug returns a valid slug. If findParent returns undefined, this could lead to a broken link. Consider adding a check or default value.

mateacademy-ai-mentor · 2025-01-06T19:53:08Z

src/components/PersonLink.tsx

+        </td>
+        <td>
+          {person.fatherName && names.includes(person.fatherName) ? (
+            <Link to={`/people/${findParent(person.fatherName)?.slug}`}>


Similar to the mother link, ensure that findParent(person.fatherName)?.slug returns a valid slug. Add a check or default value to handle cases where findParent returns undefined.

mateacademy-ai-mentor · 2025-01-06T19:53:08Z

src/components/SortLink.tsx

+    sort: field === sortField && isReversed ? null : field,
+    order: field === sortField && !isReversed ? 'desc' : null,


The params object logic for setting sort and order is incorrect. When the field is the current sort field and is reversed, order should be set to 'asc' instead of null. Similarly, ensure sort is set correctly when toggling the sort order.

mateacademy-ai-mentor · 2025-01-06T19:53:08Z

src/utils/getFilteredPeople.ts

+export interface Filter {
+  sex: string | null;
+  name: string | null;
+  centuries: string[];


Consider using a more specific type for centuries if possible, such as an array of numbers, to avoid unnecessary string conversions.

mateacademy-ai-mentor · 2025-01-06T19:53:08Z

src/utils/getFilteredPeople.ts

+    }
+
+    if (centuries.length) {
+      const personCentury = Math.ceil(person.born / 100);


Ensure that the calculation of personCentury is accurate, especially if person.born can be negative or zero. Consider adding validation or handling for such cases.

solution

b6f06e6

pogorielova reviewed Jan 6, 2025

View reviewed changes

SemenVodolazskij approved these changes Jan 6, 2025

View reviewed changes

mateacademy-ai-mentor suggested changes Jan 6, 2025

View reviewed changes

fix error

86332dd

YoNiga7 requested a review from mateacademy-ai-mentor January 6, 2025 19:50

mateacademy-ai-mentor approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

solution #1183

solution #1183

YoNiga7 commented Jan 5, 2025

pogorielova left a comment

SemenVodolazskij left a comment

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

mateacademy-ai-mentor Jan 6, 2025

		onClick={() => toggleCenturies(`${century}`)}
		params={{ centuries: toggleCenturies(`${century}`) }}

		sort: field === sortField && isReversed ? null : field,
		order: field === sortField && !isReversed ? 'desc' : null,

solution #1183

Are you sure you want to change the base?

solution #1183

Conversation

YoNiga7 commented Jan 5, 2025

pogorielova left a comment

Choose a reason for hiding this comment

SemenVodolazskij left a comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment