Skip to content

add task solution#3650

Open
nataSokolnikova wants to merge 4 commits intomate-academy:masterfrom
nataSokolnikova:develop
Open

add task solution#3650
nataSokolnikova wants to merge 4 commits intomate-academy:masterfrom
nataSokolnikova:develop

Conversation

@nataSokolnikova
Copy link
Copy Markdown

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Good effort on setting up the components! However, some changes are needed to meet the core requirements of the task.

The main issue is how props are being passed. The task requires you to pass the entire person object as a single prop, like <Person person={misha} />, instead of passing each field individually. You'll need to update App.jsx to create these objects and then adjust Person.jsx to accept a single person prop and destructure the values from it.

This also leads to a bug with the isMarried property. It's currently being passed as a string ("true"), which is always considered 'truthy' in JavaScript. This should be passed as a boolean (isMarried={true}) to make the conditional logic work correctly.

Finally, please remember to format the ternary operator in Person.jsx across multiple lines for better readability, as mentioned in the project checklist [CHECKLIST ITEM #2]. Addressing these points will also fix the failing tests.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -1 +1,13 @@
// export const Person = ({ person }) => ();
export const Person = ({ name, age, sex, isMarried, partnerName }) => (
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 component should accept a single person object as a prop, as specified in the task description: 'Pass the entire person object as a prop using <Person person={misha} />'. You should then destructure the individual properties from that person object inside the component.

{age && <p className="Person__age">I am {age}</p>}

<p className="Person__partner">
{!isMarried
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 conditional logic is not robust. Since props like isMarried are passed as strings from App.jsx (e.g., isMarried="false"), this check will produce incorrect results. !"false" evaluates to false, which would cause an unmarried person to be displayed as married. Consider passing boolean values for props like this (isMarried={false}) and ensuring your logic handles them correctly.

Comment on lines +8 to +10
{!isMarried
? 'I am not married'
: `${partnerName} is my ${sex === 'm' ? 'wife' : 'husband'}`}
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 violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. The ternary operator should be formatted across multiple lines to improve readability.

src/App.jsx Outdated
Comment on lines +7 to +15
<Person
name="Misha"
age={37}
sex="m"
isMarried="true"
partnerName="Natasha"
/>
<Person name="Olya" sex="f" isMarried="true" partnerName="Maksym" />
<Person name="Alex" age={25} sex="m" isMarried="false" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The task description requires passing the entire person object as a single prop (e.g., <Person person={misha} />), not as individual fields. You should define the misha, olya, and alex objects and pass them to the Person component.

Additionally, isMarried is being passed as a string ("true" or "false"). In JavaScript, all non-empty strings are 'truthy', which will cause the conditional logic in the Person component to always evaluate the 'married' path. This prop should be a boolean value (e.g., isMarried={true}).

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Excellent work on refactoring the components and fixing the prop-passing logic! All the core functional requirements are now met.

However, this requires changes because the automated build is failing due to a dependency conflict in your package.json file. The eslint-config-standard package requires an older version of eslint-plugin-n. To fix this, please downgrade eslint-plugin-n to ^16.0.0, delete your node_modules folder and package-lock.json file, and then run npm install.

While you're at it, please also address the code style feedback from the previous review by formatting the ternary operator in Person.jsx across multiple lines for better readability [CHECKLIST ITEM #2].


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

export const Person = ({ person }) => {
const { name, age, sex, isMarried, partnerName } = person;

const partner = sex === 'm' ? 'wife' : 'husband';
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 violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. Please format this ternary operator to span multiple lines for better readability.

{age && <p className="Person__age">I am {age}</p>}

<p className="Person__partner">
{!isMarried ? 'I am not married' : `${partnerName} is my ${partner}`}
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 violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. Please format this ternary operator across multiple lines to improve readability.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

Great job on implementing the core logic! Your Person component correctly handles all the required conditions. I am approving this solution as an educational override to help you progress, but there are a couple of important points to address for future submissions.

The main area for improvement is the code style in src/components/Person/Person.jsx. The feedback about formatting ternary operators still needs to be addressed. Please remember to place the condition, true case, and false case on separate lines to improve readability [CHECKLIST ITEM #2].

Additionally, the project setup is failing due to a dependency conflict. To fix this, please downgrade the eslint-plugin-n package in your package.json to "^16.0.0" and run npm install again. Keep up the excellent work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

export const Person = ({ person }) => {
const { name, age, sex, isMarried, partnerName } = person;

const partner = sex === 'm' ? 'wife' : 'husband';
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 violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. Please format this ternary operator across multiple lines for better readability.

{age && <p className="Person__age">I am {age}</p>}

<p className="Person__partner">
{!isMarried ? 'I am not married' : `${partnerName} is my ${partner}`}
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 violates checklist item #2: 'Format ternary operator operands correctly - move each operand to the separate line'. Please format this ternary operator across multiple lines for better readability.

Copy link
Copy Markdown

@Anton-Kuchmasov Anton-Kuchmasov left a comment

Choose a reason for hiding this comment

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

Good job!

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.

3 participants