Skip to content

react_person#3734

Open
gadyka08-lab wants to merge 4 commits into
mate-academy:masterfrom
gadyka08-lab:develop
Open

react_person#3734
gadyka08-lab wants to merge 4 commits into
mate-academy:masterfrom
gadyka08-lab:develop

Conversation

@gadyka08-lab
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 code structure is well-implemented with proper destructuring, correct ternary formatting, and appropriate class names. However, there's a critical bug causing test failure: the age display format is incorrect. The test expects the output to be "I am 37" but your component is rendering "age: 37". In your Person component, change the age paragraph from <p>age: {age}</p> to <p>I am {age}</p>. This is a functional issue that breaks the expected output format specified in the tests, so it must be fixed before approval.


✨ 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

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 implementation correctly handles all functional requirements—conditional age display, proper partner terminology based on gender, "I am not married" messaging, correct class names, destructuring for props, and passing the entire person object as a prop. However, there's a formatting issue in src/components/Person/Person.jsx with the ternary operator used for determining partner terminology. The nested ternary inside the template literal ${sex === 'f' ? 'husband' : 'wife'} is written inline on a single line. Per checklist item #2, each ternary operand (condition, true branch, false branch) should be on its own line. To fix this, you can extract the partner logic into a separate constant before the JSX return: javascript const partnerText = person.sex === 'f' ? 'husband' : 'wife'; This way the nested ternary is no longer inline, and your component will fully comply with the formatting requirements while maintaining all functional behavior.


✨ 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 +17 to +20
{/* Тернарний оператор: якщо одружений — показуємо партнера, інакше — статус */}
{isMarried
? `${partnerName} is my ${sex === 'f' ? 'husband' : 'wife'}`
: '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.

Format ternary operator operands correctly - move each operand to the separate line. This violates checklist item #2. The condition, true branch, and false branch should each be on their own line.

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

I am approving your solution with an educational override, as all functional requirements are working correctly.

What you did well:

  • All three Person components render correctly with proper destructuring
  • Partner terminology logic works properly (husband/wife based on gender)
  • Age display handles missing data correctly
  • "I am not married" messaging displays when appropriate
  • Class names match the requirements

Learning opportunity:
The nested ternary on line 19 (${sex === 'f' ? 'husband' : 'wife'}) is written inline. For better readability, consider extracting it to a constant before the return statement, like:

const partnerText = sex === 'f' ? 'husband' : 'wife';

This follows the principle that complex expressions in JSX templates are often easier to understand when broken into named variables first.

Keep up the solid work on the core functionality—you've got the logic right!


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

Footnotes

  1. Rate AI review example

Comment thread cypress.config.js
@@ -18,6 +18,7 @@ module.exports = defineConfig({
},
component: {
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 nested ternary ${sex === 'f' ? 'husband' : 'wife'} is written inline within the template literal. Per checklist item #2, each ternary operand should be on its own line. Extract the partner terminology into a separate constant before the JSX return, e.g.: const partnerText = sex === 'f' ? 'husband' : 'wife';

Comment on lines +18 to +19
{isMarried
? `${partnerName} is my ${sex === 'f' ? 'husband' : 'wife'}`
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 nested ternary ${sex === 'f' ? 'husband' : 'wife'} violates checklist item #2 - all ternary operands should be on separate lines. Extract the partner terminology into a separate constant before JSX: const partnerText = sex === 'f' ? 'husband' : 'wife'; then use it in the template: ${partnerName} is my ${partnerText}

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