Skip to content

React person#3800

Open
balakhmei wants to merge 2 commits into
mate-academy:masterfrom
balakhmei:develop
Open

React person#3800
balakhmei wants to merge 2 commits into
mate-academy:masterfrom
balakhmei:develop

Conversation

@balakhmei

Copy link
Copy Markdown

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because there are still critical checklist violations in src/components/Person/Person.jsx, which are considered blocking for this task. On the partner line, the conditional rendering for isMarried plus the nested sex === 'm' ? 'wife' : 'husband' ternary is written as one compressed JSX expression, so it violates [CHECKLIST ITEM #2]: each operand of the main ternary must be on its own line and nested ternaries should be formatted for readability. It also violates [CHECKLIST ITEM #3] because multiple conditions (isMarried and sex === 'm') are embedded directly in JSX instead of being extracted into clearly named variables.

To fix this, before the return in Person.jsx, create variables such as const partnerLabel = sex === 'm' ? 'wife' : 'husband'; and const partnerText = isMarried ? ... : ...;, and then use those variables in the JSX. Format the ternary so the ? and : branches are on separate lines, following the checklist’s “good example” for ternaries. Everything else in your solution (props destructuring, use of Person in App, handling optional age, and correct partner wording) looks good, so once you refactor this conditional rendering and formatting, the solution will fully meet both functional and style requirements.


✨ 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 thread src/components/Person/Person.jsx Outdated
Comment on lines +11 to +12
{isMarried
? `${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" because the main ternary and the nested sex === 'm' ? 'wife' : 'husband' ternary are compressed into one expression, making it hard to read. Split the operands onto separate lines as shown in the good example.

Comment thread src/components/Person/Person.jsx Outdated
Comment on lines +11 to +13
{isMarried
? `${partnerName} is my ${sex === 'm' ? 'wife' : 'husband'}`
: `I am not married`}

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 #3: "Avoid putting several cases to conditional rendering. Create separate variable for the condition." Here, conditional rendering mixes isMarried and sex === 'm' logic directly in JSX. Move this logic into one or more variables outside the JSX and reference those variables instead.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Your solution is approved because it correctly implements all the required behaviors for the Person and App components and follows the checklist rules. The App.jsx file uses the Person component three times, passing the full misha, olya, and alex objects via the person prop without altering their structure. In Person.jsx, you destructure props as required, conditionally render age only when it exists, and correctly derive partner information based on sex and marriage status, including the exact I am not married text for unmarried people. The partner-related logic is cleanly extracted into variables, and your ternary operators are formatted with each branch on its own line, which aligns with [CHECKLIST ITEM #2] and [CHECKLIST ITEM #3]. Class names like Person, Person__name, Person__age, and Person__partner match the task requirements, ensuring the expected structure and styling hooks. As a minor improvement for future tasks, explicitly mentioning in your PR that you ran npm test and used formatting tools (like Prettier) can make your submissions look even more complete. Overall, this is a solid, requirement-compliant implementation—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