Skip to content

Add Person#3655

Open
VladChudin wants to merge 3 commits intomate-academy:masterfrom
VladChudin:develop
Open

Add Person#3655
VladChudin wants to merge 3 commits intomate-academy:masterfrom
VladChudin:develop

Conversation

@VladChudin
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

Your solution correctly implements all the functional requirements, which is great! However, this submission requires changes because it violates a couple of explicit rules from the project checklist.

In src/components/Person/Person.jsx, please address the following:

  1. Incomplete Destructuring: While you've destructured the person prop, you should also destructure the properties within the person object (name, age, etc.) to make the component's dependencies explicit and improve readability. [CHECKLIST ITEM #1]
  2. Ternary Formatting: The nested ternary operator for displaying the partner's relationship (wife/husband) should be formatted with each operand on a new line, just like the outer ternary. [CHECKLIST ITEM #2]

Once these styling issues are resolved to match the project guidelines, your solution will be ready.


✨ 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

Comment on lines +1 to +4
export const Person = ({ person }) => (
<section className="Person">
<h2 className="Person__name">
{`My name is ${person.name}`}
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 #1: 'Use destructuring for getting access to values of props object'.

While you have destructured the person object from props, the checklist encourages taking it a step further. You should also destructure the properties you need from the person object itself. This makes the code cleaner and more explicit about which fields from the person object are being used in the component.

For example:

export const Person = ({ person }) => {
  const { name, age, sex, isMarried, partnerName } = person;
  // ...then use `name`, `age`, etc. directly in your JSX
};

<p className="Person__partner">
{person.isMarried
? `${person.partnerName} is my ${
person.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'.

For better readability, this nested ternary operator should be formatted across multiple lines, similar to how you've formatted the outer ternary operator.

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

Thank you for your submission. While the application's functionality is correct, changes are needed because the code style feedback from the previous review has not been applied. These are required parts of the task as outlined in the checklist.

In src/components/Person/Person.jsx, please address the following:

  1. Destructuring: The properties inside the person object (like name, sex, isMarried) must be destructured. This makes the component's dependencies clearer and improves readability by removing the person. prefix. [CHECKLIST ITEM #1]
  2. Ternary Formatting: The nested ternary operator for displaying wife or husband must be formatted with each part on a new line to improve readability, just like the outer one. [CHECKLIST ITEM #2]

Please apply these changes to align the code with the project's style guidelines.


✨ 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,15 @@
// export const Person = ({ person }) => ();
export const Person = ({ person }) => (
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 #1: 'Use destructuring for getting access to values of props object'.

While you have destructured person from props, the next step is to destructure the properties within the person object (e.g., name, sex, isMarried). This makes the component's dependencies explicit and improves readability in the JSX by removing the person. prefix.


<p className="Person__partner">
{person.isMarried
? `${person.partnerName} is my ${person.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 nested ternary operator person.sex === 'm' ? 'wife' : 'husband' should be formatted across multiple lines for better readability, just like the outer one.

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 this iteration! You've successfully addressed all the feedback from the previous review, bringing the code perfectly in line with the project's style guidelines. The destructuring is correctly implemented and the ternary operators are well-formatted for readability. Your solution now fully meets all requirements, so I am happy to approve it. Well done!


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

Footnotes

  1. Rate AI review example

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.

2 participants