Skip to content

Implement Person component#3790

Open
Annybianzinny wants to merge 3 commits into
mate-academy:masterfrom
Annybianzinny:master
Open

Implement Person component#3790
Annybianzinny wants to merge 3 commits into
mate-academy:masterfrom
Annybianzinny:master

Conversation

@Annybianzinny

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 some core functional requirements are currently not met, and tests for the Person component are failing. The component’s rendered text does not match the expected format: for example, tests expect My name is Misha but your component renders just Misha, expects I am 37 but gets 37, and expects partner lines like Natasha is my wife / Maksym is my husband but receives wife: Natasha / husband: Maksym instead. These are critical, blocking issues because they directly contradict the task’s output specification and cause multiple unit tests in src/Person.spec.jsx and one test in src/page.spec.js to fail.

To fix this, update src/components/Person/Person.jsx so that:

  • The name line is rendered as My name is {name} inside the appropriate element.
  • The age (when present) is rendered as I am {age}.
  • The partner line (when partnerName exists) is rendered as {partnerName} is my wife for sex === 'm' and {partnerName} is my husband for sex === 'f'.
    Also, simplify the conditional logic so you avoid nested ternaries inside JSX: compute variables like partnerLabel and final text strings above the return and then render those variables. Your overall structure, prop usage, and optional age handling are on the right track, and with these string-format fixes and a small cleanup of the conditional logic, your solution should pass the tests and fully meet the 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

@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 is still a checklist-related formatting issue that’s considered part of the required standards. Functionally, your solution looks solid: App correctly uses Person three times, passes the full person objects, and Person renders the name, optional age, and partner text exactly as specified. However, in src/App.jsx you still have a nested ternary operator written on a single line, which goes against the explicit guideline that ternary operands must be split across separate lines [CHECKLIST ITEM #2].

You should rewrite that ternary so that the ? and : branches each start on their own line, similar to how you’ve formatted the ternary in Person.jsx. For example:

const value = condition
  ? firstOption
  : secondOption;

Once you adjust that ternary formatting in App.jsx, your solution will meet both the functional and style/checklist requirements. You’re very close—fixing this small formatting issue will complete the task fully.


✨ 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

@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. You correctly implemented the Person component so that it renders the name, conditionally shows the age only when it exists, and displays the partner text based on sex and isMarried, including the “I am not married” case. In App.jsx, you now use Person three times and pass the full person objects (person={misha}, person={olya}, person={alex}) while keeping the original constants unchanged, which matches the requirements. The class names (Person, Person__name, Person__age, Person__partner) are used as requested, and your conditional logic is clear and easy to read. You also followed the checklist by destructuring props in Person [CHECKLIST ITEM #1] and formatting the ternary across multiple lines [CHECKLIST ITEM #2]. Overall, this is a clean, requirement-compliant solution—well done improving it and paying attention to both functionality and style.


✨ 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