add task solution #1147

DariaNastas · 2024-12-02T13:04:53Z

mateacademy-ai-mentor

Thank you for your hard work on this pull request! Your implementation is quite comprehensive and meets many of the task requirements. However, there are a few critical issues that need to be addressed before we can approve the changes:

Typo in Function Name: The function peopleWithPerents has a typo in its name. It should be corrected to peopleWithParents to maintain clarity and consistency with naming conventions.
Sort Icon Logic: The class name for the sort icon should accurately reflect the current sortOrder. Ensure that sortOrder is properly initialized and updated, and consider using a default icon state when sortOrder is null to avoid confusion.
Sorting Logic for Null/Undefined Values: The current sorting logic returns 0 if either aValue or bValue is null or undefined, which can lead to unexpected behavior. Consider explicitly handling these cases by placing null or undefined values at the end of the sorted list.
Sort Order Logic: The sortOrder logic currently reverses the array if sortOrder is truthy, which might not behave as expected if sortOrder is not explicitly set to 'asc' or 'desc'. Ensure that specific values are checked to apply the intended sort order.
Century Filter Update: The toggleCenturyFilter function might not correctly update the params in the SearchLink component. Store the result in a variable before passing it to params to ensure the state is correctly updated.

Once these issues are resolved, your code will be in great shape! Keep up the excellent work, and feel free to reach out if you have any questions or need further clarification. 😊

✨ 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 · 2024-12-02T13:07:21Z

src/components/PeopleFilters.tsx

+  const toggleCenturyFilter = (century: string) => {
+    return centuries.includes(century)
+      ? centuries.filter(cent => cent !== century)
+      : [...centuries, century];
+  };


The toggleCenturyFilter function is designed to toggle the presence of a century in the centuries array. However, when updating the params in the SearchLink component (line 88), it might not correctly reflect the toggled state because it directly uses the return value of toggleCenturyFilter. Consider storing the result in a variable before passing it to params to ensure the state is correctly updated.

mateacademy-ai-mentor · 2024-12-02T13:07:21Z

src/components/PeoplePage.tsx

+    if (sortOrder) {
+      filteredPeople.reverse();
+    }


The sortOrder logic currently reverses the array if sortOrder is truthy. This could lead to unexpected behavior if sortOrder is not explicitly set to a meaningful value (e.g., 'asc' or 'desc'). Consider checking for specific values to ensure the sort order is applied as intended.

mateacademy-ai-mentor · 2024-12-02T13:07:21Z

src/components/PeopleTable.tsx

+      if (
+        aValue === null ||
+        aValue === undefined ||
+        bValue === null ||
+        bValue === undefined
+      ) {
+        return 0;


The sorting logic currently returns 0 if either aValue or bValue is null or undefined. This could lead to unexpected sorting behavior. Consider handling these cases more explicitly, perhaps by placing null or undefined values at the end of the sorted list.

mateacademy-ai-mentor · 2024-12-02T13:07:21Z

src/components/PeopleTable.tsx

                <span className="icon">
-                  <i className="fas fa-sort" />
+                  <i
+                    className={`fas fa-sort-${sortOrder === 'asc' ? 'up' : 'down'}`}


The class name for the sort icon is determined by the sortOrder. Ensure that the icon correctly reflects the current sort order by verifying that sortOrder is properly initialized and updated. Consider using a default icon state when sortOrder is null.

mateacademy-ai-mentor · 2024-12-02T13:07:21Z

src/utils/PeopleWithParents.ts

@@ -0,0 +1,27 @@
+import { Person } from '../types';
+
+export const peopleWithPerents = (peopleFromServer: Person[]): Person[] => {


There is a typo in the function name peopleWithPerents. It should be corrected to peopleWithParents to match the intended meaning and maintain consistency with the naming conventions.

mateacademy-ai-mentor

Your code has been reviewed and is approved! 🎉 While there are some areas for improvement, such as handling null or undefined values in sorting, ensuring consistent navigation with react-router-dom, and refining the toggleCenturyFilter logic, you've made significant progress. Addressing these issues will enhance your code's robustness and maintainability. Keep up the great work, and continue learning from these experiences to refine your skills further!

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

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeopleFilters.tsx

+  const toggleCenturyFilter = (century: string) => {
+    return centuries.includes(century)
+      ? centuries.filter(cent => cent !== century)
+      : [...centuries, century];


The toggleCenturyFilter function currently returns a new array of centuries but does not update the search parameters directly. Consider updating the search parameters within this function or ensure that the returned value is used to update the state elsewhere.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeopleFilters.tsx

-              16
-            </a>
+            {centuriesList.map(century => {
+              const updatedCenturies = toggleCenturyFilter(century);


The toggleCenturyFilter function is called here, but its result is not used to update the search parameters. Ensure that the updated centuries list is passed to the params prop of SearchLink to reflect changes in the URL.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeopleFilters.tsx

-        <a className="button is-link is-outlined is-fullwidth" href="#/people">
+        <SearchLink
+          className="button is-link is-outlined is-fullwidth"
+          onClick={resetFilters}


The onClick handler for SearchLink does not need to be used here since SearchLink will automatically handle navigation based on the params prop. Consider removing the onClick handler to avoid potential conflicts.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeoplePage.tsx

+import { getPeople } from '../api';
+import { useSearchParams, useParams } from 'react-router-dom';
+import { SortBy } from '../types/SortBy';
+import { peopleWithParents } from '../utils/PeopleWithParents';


There is a typo in the function name peopleWithParents. It should be corrected to peopleWithParents to maintain clarity and consistency with naming conventions.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeoplePage.tsx

+    if (sortBy && sortMethods[sortBy]) {
+      filteredPeople.sort(sortMethods[sortBy]);


The sorting logic does not handle null or undefined values explicitly. Consider placing null or undefined values at the end of the sorted list to avoid unexpected behavior.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeoplePage.tsx

+  const sex = searchParams.get('sex') || '';
+  const centuries = searchParams.getAll('centuries') || [];
+  const sortBy = searchParams.get('sort') as SortBy | null;
+  const sortOrder = searchParams.get('order');


The sortOrder logic currently reverses the array if sortOrder is 'desc'. Ensure that specific values are checked to apply the intended sort order, and consider initializing sortOrder to a default value if it's null.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeopleTable.tsx

+                  sort: 'name',
+                  order: sortOrder === 'asc' ? 'desc' : 'asc',
+                }}
+                onClick={() => handleSort('name')}


The onClick handler for SearchLink is redundant since SearchLink will automatically handle navigation based on the params prop. Consider removing the onClick handler to avoid potential conflicts.

mateacademy-ai-mentor · 2024-12-02T14:22:09Z

src/components/PeopleTable.tsx

+      const aValue = a[sortField as keyof Person];
+      const bValue = b[sortField as keyof Person];


The sorting logic for aValue and bValue should ensure that both values are comparable. Consider adding a check to handle cases where aValue or bValue might not be directly comparable, such as when they are objects or other non-primitive types.

mateacademy-ai-mentor · 2024-12-02T14:22:10Z

src/components/PeopleTable.tsx

+          >
+            <td>
+              <a
+                href={`#/people/${person.slug}`}


Using href with a hash (#) for navigation might not be ideal if you are using react-router-dom. Consider using Link from react-router-dom for navigation to ensure consistency with the rest of the application.

mateacademy-ai-mentor · 2024-12-02T14:22:10Z

src/utils/PeopleWithParents.ts

+    mother: parentsMap.mothers[person.motherName || ''] || null,
+    father: parentsMap.fathers[person.fatherName || ''] || null,


The use of person.motherName || '' and person.fatherName || '' is a bit redundant since parentsMap keys are strings. Consider directly using person.motherName and person.fatherName without the fallback to an empty string, as the map will handle undefined keys by returning undefined, which is then correctly defaulted to null.

DariaNastas added 5 commits November 28, 2024 16:44

add task solution

3a272b7

add task solution

a702622

add task solution

cb8d5c0

add task solution

b9821b9

add task solution

b4d9fa1

mateacademy-ai-mentor suggested changes Dec 2, 2024

View reviewed changes

add task solution

c35fbc3

DariaNastas requested a review from mateacademy-ai-mentor December 2, 2024 14:19

mateacademy-ai-mentor approved these changes Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add task solution #1147

add task solution #1147

DariaNastas commented Dec 2, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

mateacademy-ai-mentor Dec 2, 2024

		@@ -0,0 +1,27 @@
		import { Person } from '../types';

		export const peopleWithPerents = (peopleFromServer: Person[]): Person[] => {

		if (sortBy && sortMethods[sortBy]) {
		filteredPeople.sort(sortMethods[sortBy]);

		const aValue = a[sortField as keyof Person];
		const bValue = b[sortField as keyof Person];

		mother: parentsMap.mothers[person.motherName \|\| ''] \|\| null,
		father: parentsMap.fathers[person.fatherName \|\| ''] \|\| null,

add task solution #1147

Are you sure you want to change the base?

add task solution #1147

Conversation

DariaNastas commented Dec 2, 2024

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

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