Develop #1136

italomagno · 2024-11-22T10:36:39Z

No description provided.

…h rules

mateacademy-ai-mentor

Thank you for your hard work on this Pull Request! While your code is mostly well-structured and meets the task requirements, there are a few critical issues that need to be addressed before we can approve it:

Sorting Logic: The handleSortOrder function always sets the order to desc when the sort is initially applied. Please ensure this aligns with the desired behavior for sorting when no order is specified.
Error Handling: The error state is set based on the length of filteredPeople. Make sure this logic is correct and that filteredPeople is updated appropriately before this check to avoid incorrect error messages.
Redundant Code: The setIsLoading(false); call in the finally block is redundant since it's already called in both the then and catch blocks. Please remove it from the finally block.
State Management: The setFilteredPeople function might lead to duplicated entries if the effect runs multiple times. Consider setting filteredPeople directly to response instead of spreading prev.
Century Handling: Ensure that the logic for updating centuries in the handleAddNewCentury function is correctly handled in the getSearchWith function and that the returned value is used appropriately.
Typo: Correct the typo in the interface name ErrrorComponentProps to ErrorComponentProps for consistency.
Routing Logic: The nested route inside /people is redundant. Consider handling the logic for rendering different content based on the presence of :slug within the PeoplePage component itself.

Once these issues are resolved, your code will be in great shape! Keep up the good 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-11-22T10:38:41Z

src/App.tsx

+        <Route path="/people" element={<PeoplePage />}>
+          <Route path="/people/:slug" element={<PeoplePage />} />
+        </Route>


The nested route inside /people is redundant since it uses the same component PeoplePage. If PeoplePage is supposed to handle both /people and /people/:slug, you might want to handle the logic for rendering different content based on the presence of :slug within the PeoplePage component itself. Consider removing the nested route or adjusting the component logic.

mateacademy-ai-mentor · 2024-11-22T10:38:41Z

src/components/ErrorComponent.tsx

+import React from 'react';
+import { ErrorMessages } from './PeoplePage';
+
+interface ErrrorComponentProps {


There is a typo in the interface name ErrrorComponentProps. It should be corrected to ErrorComponentProps to maintain consistency and avoid confusion.

mateacademy-ai-mentor · 2024-11-22T10:38:41Z

src/components/PeopleFilters.tsx

+      ? centuries.filter(c => c !== century.toString())
+      : [...centuries, century.toString()];
+
+    return newCenturies;


The handleAddNewCentury function returns newCenturies, but this value is not used directly in the component. Instead, it's passed to getSearchWith in the to prop of the Link component. Ensure that the logic for updating the centuries is correctly handled in the getSearchWith function and that the returned value is used appropriately.

mateacademy-ai-mentor · 2024-11-22T10:38:41Z

src/components/PeoplePage.tsx

+        }
+
+        setPeople(response);
+        setFilteredPeople(prev => [...prev, ...response]);


The setFilteredPeople function is called with a function that spreads the previous state and adds the response. This might lead to duplicated entries if the effect runs multiple times. Consider setting filteredPeople directly to response instead of spreading prev.

mateacademy-ai-mentor · 2024-11-22T10:38:41Z

src/components/PeoplePage.tsx

+        setIsLoading(false);
+      })
+      .finally(() => {
+        setIsLoading(false);


The setIsLoading(false); call in the finally block is redundant since it's already called in both the then and catch blocks. You can remove it from the finally block.

mateacademy-ai-mentor · 2024-11-22T10:38:41Z

src/components/PeoplePage.tsx

+    if (filteredPeople.length === 0) {
+      setError(ErrorMessages.NO_PEOPLE_MATCHING_CRITERIA);
+    } else {
+      setError(ErrorMessages.BLANK);
+    }


The error state is set based on the length of filteredPeople. Ensure that this logic is correct and that filteredPeople is updated appropriately before this check. Otherwise, it might lead to incorrect error messages.

mateacademy-ai-mentor · 2024-11-22T10:38:42Z

src/components/PeopleTableHeaders.tsx

+      case sort && sort === label.toLowerCase() && !order:
+        return getSearchWith(searchParams, {
+          sort: label.toLowerCase(),
+          order: Order.desc,
+        });


In the handleSortOrder function, the first case checks if sort is equal to label.toLowerCase() and order is falsy. This will always set the order to desc when the sort is initially applied. Ensure this logic aligns with the desired behavior for sorting when no order is specified.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: 7]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: 7]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

mateacademy-ai-mentor

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
_{[CODE: 7]}

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

raulriato

Hey, @italomagno. It seems you forgot to run the deploy for your project. Please do it so I can review it

italomagno · 2024-11-23T10:02:06Z

This is the deployed

julianFBarbosa · 2024-11-23T13:18:44Z

src/components/ErrorComponent.tsx

+export function ErrorComponent({ message }: ErrorComponentProps) {
+  return (
+    <>
+      {message === ErrorMessages.SOMETHING_WENT_WRONG && (


The way you implemented this isn't wrong, but i would recommend to use dynamic property access to avoid repetition (and personally i think that's also improves code readability)!

Imagine if we have 50 types of error messages, what would we do?

an example implementation:

<> {message && ( <p data-cy={`${message}Message`}> {ErrorMessages[message]} </p> )} </>

julianFBarbosa · 2024-11-23T13:23:19Z

src/components/Navbar.tsx

@@ -8,13 +13,16 @@ export const Navbar = () => {
    >
      <div className="container">
        <div className="navbar-brand">
-          <a className="navbar-item" href="#/">
+          <a
+            className={`navbar-item ${path.includes('home') || path === '/' ? 'has-background-grey-lighter' : ''}`}


Suggested change

className={`navbar-item ${path.includes('home') || path === '/' ? 'has-background-grey-lighter' : ''}`}

className={`navbar-item ${path.includes('home') || path === '/' ? 'has-background-grey-lighter' : ''}`}

We can extract it to a variable (or a function) to avoid complexity in the JSX =)

const getHomeLinkClassNames = () => { const isHome = path.includes('home') || path === '/' return isHome ? 'has-background-grey-lighter' : '' } ... <a className={getHomeLinkClassNames()}>

julianFBarbosa · 2024-11-23T13:27:57Z

src/components/PeopleFilters.tsx

 export const PeopleFilters = () => {
+  const availableCenturies = [16, 17, 18, 19, 20];


Suggested change

const availableCenturies = [16, 17, 18, 19, 20];

const availableCenturies = [16, 17, 18, 19, 20];

We can move this variable to outside of the component, since we aren't changing it's values anywhere in the code flow, so we can avoid it's re-creation across re-renders =)

julianFBarbosa · 2024-11-23T13:30:54Z

src/components/PeoplePage.tsx

+      setFilteredPeople(prev => {
+        return [...prev].filter(
+          person =>
+            person.name.toLowerCase().includes(query.toLowerCase()) ||
+            (person.mother &&
+              person.mother.name.toLowerCase().includes(query.toLowerCase())) ||
+            (person.father &&
+              person.father.name.toLowerCase().includes(query.toLowerCase())),
+        );
+      });


Suggested change

setFilteredPeople(prev => {

return [...prev].filter(

person =>

person.name.toLowerCase().includes(query.toLowerCase()) ||

(person.mother &&

person.mother.name.toLowerCase().includes(query.toLowerCase())) ||

(person.father &&

person.father.name.toLowerCase().includes(query.toLowerCase())),

);

});

setFilteredPeople(prev => {

return [...prev].filter(

person =>

person.name.toLowerCase().includes(query.toLowerCase()) ||

(person.mother &&

person.mother.name.toLowerCase().includes(query.toLowerCase())) ||

(person.father &&

person.father.name.toLowerCase().includes(query.toLowerCase())),

);

});

I would recommend to use the classical if/else here, it would improve readability and facilitate to add new filters in the future =)

julianFBarbosa

I left some comments and suggestions.

But the main target is well done: the app is working the same way as the example!

Congrats =)

julianFBarbosa

nice job =)

italomagno added 3 commits November 20, 2024 19:06

creating components, and ading PeopleTableHeaders to manipulate searc…

66719aa

…h rules

need adjustments on filter

fffd0e8

finishing the challenge

d81a8e2

mateacademy-ai-mentor suggested changes Nov 22, 2024

View reviewed changes

mateacademy-ai-mentor reviewed Nov 22, 2024

View reviewed changes

finishing again

682b83b

italomagno requested a review from mateacademy-ai-mentor November 22, 2024 22:42

raulriato suggested changes Nov 22, 2024

View reviewed changes

finishing readme

62058a2

italomagno requested a review from raulriato November 23, 2024 10:02

julianFBarbosa reviewed Nov 23, 2024

View reviewed changes

julianFBarbosa suggested changes Nov 23, 2024

View reviewed changes

italomagno requested a review from julianFBarbosa November 23, 2024 14:08

changes

e90f94f

julianFBarbosa approved these changes Nov 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #1136

Develop #1136

italomagno commented Nov 22, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor Nov 22, 2024

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor left a comment

raulriato left a comment

italomagno commented Nov 23, 2024

julianFBarbosa Nov 23, 2024

julianFBarbosa Nov 23, 2024

julianFBarbosa Nov 23, 2024

julianFBarbosa Nov 23, 2024

julianFBarbosa left a comment

julianFBarbosa left a comment

	className={`navbar-item ${path.includes('home') \|\| path === '/' ? 'has-background-grey-lighter' : ''}`}
	className={`navbar-item ${path.includes('home') \|\| path === '/' ? 'has-background-grey-lighter' : ''}`}

		export const PeopleFilters = () => {
		const availableCenturies = [16, 17, 18, 19, 20];

Develop #1136

Are you sure you want to change the base?

Develop #1136

Conversation

italomagno commented Nov 22, 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

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

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

raulriato left a comment

Choose a reason for hiding this comment

italomagno commented Nov 23, 2024

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

julianFBarbosa left a comment

Choose a reason for hiding this comment

julianFBarbosa left a comment

Choose a reason for hiding this comment